Manually optimize a rem 64 instruction to avoid regression on Mono #96203
Conversation
…s which do not currently optimize it
ghost
added
area-System.Collections
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-collections Issue Detailsdotnet/perf-autofiling-issues#26185 (comment) cc @adamsitnik as requested
|
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
The changes LGTM, big thanks for such a quick fix @andrewjsaid !
@kotlarmilos @matouskozak could you please verify it and merge the PR if it solves the problem?
FWIW I'll be back to work at the beginning of January so I won't be able to respond until then.
adamsitnik
added
tenet-performance
@andrewjsaid Thank you for taking swift action. After using the entire expression, like |
|
Glad to hear that, @kotlarmilos ! Apologies for all this in the first place 🙈 |
| { | ||
| if (key.Length < minLength) minLength = key.Length; | ||
| if (key.Length > maxLength) maxLength = key.Length; | ||
| lengthFilter |= (1UL << (key.Length % 64)); |
There was a problem hiding this comment.
Seems like something that should be fixed in mono itself, if it makes such an impactful difference?
There was a problem hiding this comment.
This doesn't appear to be the primary cause of regressions in c28bec4. We will investigate it further to detect the root cause.
There was a problem hiding this comment.
Was the validation in #96203 (comment) incorrect? Can we revert this then?
There was a problem hiding this comment.
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 in System.Collections.Perf_SubstringFrozenDictionary on mono dotnet/perf-autofiling-issues#26221.
This is strange as the original commit c28bec4 is designed to not affect TryGetValue on substring strategy subtypes of OrdinalStringFrozenDictionary . It works that way because each concrete implementation should be getting it's own codegen and in turn be optimized to if(true) as this existing comment sums up
Based on that I'd say regressions on SubstringFrozenDictionary tests point towards the method call to CheckLengthFilter is not being inlined or possibly we are even doing virtual method dispatch.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's also worth noting that x % 64 becoming x & 0x3F is only a valid optimization for positive x, while it returns a different result for negative x, so it's not always a universal option to replace either.
This optimization currently lights up in RyuJIT by virtue of key being a string and the runtime having implicit knowledge that string.Length (as well as array.Length and span.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 for x % y, x is positive and y 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 since CheckLengthQuick was introduced? 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?
There was a problem hiding this comment.
Could the regressions be related to inlining, especially since CheckLengthQuick was introduced?
Almost certainly since in CoreCLR, for the benchmark SubstringFrozenDictionary, if(CheckLengthQuick(key)) is first inlined to if(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 concrete sealed 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's Equals, GetHashCode as it is generating the code for the specific implementation not the base class. in CoreCLR the same is happening for CheckLengthQuick, however as opposed to the existing methods, CheckLengthQuick is virtual and not overridden - could either of these be a reason that mono isn't inlining it like it presumably inlines Equals and GetHashCode?
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 the if (CheckLengthQuick(key)) and some indenting.
… runtimes which do not currently optimize it (dotnet#96203)" This reverts commit bc83100.
5 participants
dotnet/perf-autofiling-issues#26185 (comment)
cc @adamsitnik as requested