[5.4] - RavenDB-24810 & RavenDB-24815

[grisha-kotler]

Consists of 2 changes:

• High usage of LOH memory - Replace arrays used in the term cache and for sorting that exceed 64KB with unmanaged arrays. This reduces the LOH size significantly. Although this cache is long-lived, it can be disposed of when no longer needed, which would leave gaps in the LOH and cause fragmentation. Moving to unmanaged memory eliminates this fragmentation risk entirely.
  https://issues.hibernatingrhinos.com/issue/RavenDB-24810/High-usage-of-LOH-memory-in-Lucene

• Reduce object reference overhead - Store field numbers (int) in arrays instead of field name strings. This reduces the number of objects that the GC needs to traverse, thereby significantly reducing Gen 2 GC time.
  https://issues.hibernatingrhinos.com/issue/RavenDB-24815/Large-reference-graph-for-arrays-of-strings

[ayende]

Why don't make everything unmanaged?

[grisha-kotler]

I think using the ArrayPool will be more cost-effective for small allocations. For long arrays, using this threshold will accommodate 8,192 elements, while for TermInfo (which is 32 bytes), it will contain up to 2,048 elements. Smaller segments with fewer cached terms are more likely to be merged frequently than larger segments, which creates more allocation and deallocation cycles.

[ayende]

The problem here is that you looking at memory only, but the virtual method calls, etc are also costly.
why not do everything in unmanaged?

[grisha-kotler]

I removed the virtual method calls.

[ayende]

And you added two branches for each index? And if statemen ton each op.
Why is that something that we want to do?

[ppekrol]

This is sth that we can check, but in general I think that calling AllocHGlobal and FreeHGlobal for each of those each time will have a much bigger penalty. Smaller allocations should be managed and come from a pool IMO in this case.

[arekpalinski]

Copying data between managed and unmanaged memory isn't for free. Also we have more context here as @grisha-kotler pointed in this first answer. So for me it seems reasonable to use unmanaged only for bigger allocations (since we're trying to solve LOH compaction issue).

Regarding additional if making two branches, sometimes we tried to optimize code to make it branchless but not always it resulted in better performance.

I think the decision should be made based on perf testing results instead of relaying on intuition. It's too important code path. Note that it will require quite comprehensive testing, with variety of different scenarios.

[maciejaszyk]

We could avoid boxing here by using TArray where TArray : IArray<int>

and during construction:

			if (arr.GetType() == typeof(ManagedArray<int>))
				return new AnonymousClassDocValues<ManagedArray<int>>(end, (ManagedArray<int>)arr, this);
			
			return new AnonymousClassDocValues<UnmanagedArray<int>>(end, (UnmanagedArray<int>)arr, this);

[arekpalinski]

While agree this is good and beneficial to move it to unmanaged memory, I'd like to make a note here for us to consider. I think this is big change for us which potentially might be source of different problems.

I'm thinking about potential issues related to:

• unmanaged heap fragmentation
• long lived unmanaged allocations - since it's going to be kept by a cache

[grisha-kotler]

I've introduced a UseOnlyManagedArray flag to give us precise control over this behavior and mitigate the risks.

For 32-bit: You are absolutely right. Unmanaged heap fragmentation can become a serious issue due to the limited virtual address space. To prevent this, we will set UseOnlyManagedArray = true in our 32-bit builds, completely avoiding unmanaged allocations and the associated risks.
For 64-bit: This is far less of a concern. The vast virtual address space in 64-bit processes makes it highly unlikely for fragmentation to cause allocation failures. The performance benefits of avoiding the LOH outweigh the negligible risk of fragmentation here.

Regading the long-lived unmanaged allocations, this is a deliberate trade-off. While these allocations might be long-lived because they are held by a cache, it is preferable to have them in the unmanaged heap rather than the LOH.

[arekpalinski]

Copying data between managed and unmanaged memory isn't for free. Also we have more context here as @grisha-kotler pointed in this first answer. So for me it seems reasonable to use unmanaged only for bigger allocations (since we're trying to solve LOH compaction issue).

Regarding additional if making two branches, sometimes we tried to optimize code to make it branchless but not always it resulted in better performance.

I think the decision should be made based on perf testing results instead of relaying on intuition. It's too important code path. Note that it will require quite comprehensive testing, with variety of different scenarios.

[arekpalinski]

Previously we called it before actually releasing other resources but now we do it before. It that okay? (just sanity check, I don't know who uses OnArrayHolderDisposed)

[grisha-kotler]

OnArrayHolderDisposed is used for monitor the managed memory usage.
I moved it after the disposal.

[arekpalinski]

OK

[arekpalinski]

Can we just pass _stringIndexCache, _longCache, _doubleCache and dispose them explicitly there? So this way we won't need to allocate and use Action.

DisposableAction is private so we can make it's implementation more specific to what we deal with here.

[arekpalinski]

What I meant here was to have DisposableAction(StringIndexCache stringIndexCache, LongCache longCache, DoubleCache doubleCache) instead of DisposableAction(Action action) so we don't need to do the allocation of Action

[grisha-kotler]

done