Seems like something that should be fixed in mono itself, if it makes such an impactful difference?

This doesn't appear to be the primary cause of regressions inc28bec4. We will investigate it further to detect the root cause.

Was the validation in#96203 (comment) incorrect? Can we revert this then?

From my limited knowledge and research I couldn't find the optimization of% 64 ->& 0x3F in mono's code so this optimization might still be valid.

Just looking at the regressions and I want to point out that we see a regression inSystem.Collections.Perf_SubstringFrozenDictionary on monodotnet/perf-autofiling-issues#26221.

This is strange as the original commitc28bec4 is designed to not affectTryGetValue onsubstring strategy subtypes ofOrdinalStringFrozenDictionary . It works that way because each concrete implementation should be getting it's own codegen and in turn be optimized toif(true) as this existing comment sums up

Based on that I'd say regressions onSubstringFrozenDictionary tests point towards the method call toCheckLengthFilter is not being inlined or possibly we are even doing virtual method dispatch.

so this optimization might still be valid.

Even if it is, the changed code is harder to understand / maintain than the original IMO, and the pattern of mod'ing an array/span length is super common; this is just one occurrence of that. If it's impactful here, it'd be impactful in many more places, and I'd prefer we not one-off it. It also sounds like the measurements that suggested this was valuable in this case was flawed, and so we don't actually know in this particular case whether it made a meaningful difference.

It's also worth noting thatx % 64 becomingx & 0x3F is only a valid optimization forpositive x, while it returns a different result fornegative x, so it's not always a universal option to replace either.

This optimization currently lights up in RyuJIT by virtue ofkey being astring and the runtime having implicit knowledge thatstring.Length (as well asarray.Length andspan.Length) are never negative.

From my limited knowledge and research I couldn't find the optimization of % 64 -> & 0x3F in mono's code so this optimization might still be valid.

Such optimizations generally involve checking that forx % y,x is positive andy is a power of two. It's generally easiest to just check the codegen, but this is really one of those fundamental optimizations around division/remainder that a compiler should recognize as Stephen indicated.

Happy to submit a revert PR now and for it to be approved/merged whenever but I might be unavailable for a few weeks at some point soon so I'd rather submit now.

Was the validation in#96203 (comment) incorrect? Can we revert this then?

The validation was incorrect, sorry for confusion.

Could the regressions be related to inlining, especially sinceCheckLengthQuick was introduced? Additionally, there has been a change from usingif (Equals(item, _items[index])) toif (hashCode == _hashTable.HashCodes[index]). Is it possible thatEquals has been optimized?

Could the regressions be related to inlining, especially since CheckLengthQuick was introduced?

Almost certainly since in CoreCLR, for the benchmarkSubstringFrozenDictionary,if(CheckLengthQuick(key)) is first inlined toif(true) and then just optimized away resulting in no change at all. Whereas there is a regression in mono meaning that at the very least one of these optimizations didn't work.

The inlining is possible because concretesealed implementations of FrozenDictionary all have this line of code

private protected override ref readonly TValue GetValueRefOrNullRefCore(string key) => ref base.GetValueRefOrNullRefCore(key);

which allows the JIT to codegen for each concrete implementation. When doing so, it is able to inline the implementation'sEquals,GetHashCode as it is generating the code for the specific implementation not the base class. in CoreCLR the same is happening forCheckLengthQuick, however as opposed to the existing methods,CheckLengthQuick isvirtual andnot overridden - could either of these be a reason that mono isn't inlining it like it presumably inlinesEquals andGetHashCode?

Additionally, there has been a change from using if (Equals(item, _items[index])) to if (hashCode == _hashTable.HashCodes[index]). Is it possible that Equals has been optimized?

@kotlarmilos I believe that's just the diff viewer. The change is really just adding theif (CheckLengthQuick(key)) and some indenting.