The HashSet in the Hot Spot
The title of this post reminds me of a Bones episode. This blog post is actually from an internal mail made by Federico Lois, as part of bigger perf work.
Here is the calling code, note that this is typically called many times, and _pageStates is a HashSet<PagerState>.
The following change was made:
And here are the results:
That is 230% improvement! I can identify with the sentiment.
Comments
When providing that kind of a performance gain, you should provide a distribution of the values as well. A reader could take it as the best optimization to perform on any set of HashedSet.Add operations. In your case, states are probably repeated very frequently, hence the performance gain.
I take it this code isn't supposed to be thread-safe?
What's the point of using HashSet<T> where T does not implement GetHashCode or any specific equality stuff?
@Mike it does have an special purpose comparer passed in the constructor. Aka HashSet<T> cannot be improved much more than it's current state. <br/><br/> @Thomas, no. The code is not thread-safe by design. Remember that given the highly threaded nature of RavenDB most of the code is in itself single-threaded with very minimal (we make special enfasis in MINIMAL) amount of shared state. <br/><br/> @Scooletz. Noone can apply this optimization without having a clear understanding of the distribution of the calls. Distribution always matter, I am currently applying an optimization on another low-level routine in 4.0 that gets us 3x improvement just because we can assume without any danger that there is certain locality based distribution of calls. In real life that means that our benchmark of Voron that took 4 minutes is now taking 3:30 (it shaved 30 seconds out of the whole benchmark).
Wouldn't a better GetHashCode() implementation make it faster?
@Dmitry nope, already overloaded with a very fast GetHashCode() implementation... even inlined in the specialized comparer.
Scooletz, If some reader is going to do blind performance optimizations without stopping to understand the reasoning behind that, they have already lost.
Thomas, No, it isn't. HashSet<T> isn't thread safe either, mind.
@Federico, are we talking about the same code here?<br><br> https://github.com/ayende/ravendb/blob/v4.0/src/Voron/Impl/LowLevelTransaction.cs#L37<br> https://github.com/ayende/ravendb/blob/v4.0/src/Voron/Impl/PagerState.cs
@Mike, mind you I missed that one. Several months ago (late last year) we did a pass to add customized versions for all the ones that showed at profiling runs, we may have been missed that one (or it wasnt an issue back then :D). <br/>
Either way, we have measured the cost the HashSet<T> when you have a known distribution and it is still way faster to run a comparizon than access the HashSet<T>. I have added now the specialized one on pager state to handle the cases we are not hitting with the first (Which with todays number is aproximately 1 in 3). <br/>
Measurements after the change show an improvement of 20%, which is consistent with our measurements. Very far from the 2.3X improvement won exploiting the call distribution. See: https://onedrive.live.com/redir?resid=A62AB07630D4672D!83927&authkey=!ADY_HIeyelsWIp0&v=3&ithint=photo%2cpng <br/>
Either way, thanks for spotting that one; we just added 20% on top of the 2.3X in a very hot hot-spot. <br/>
Comment preview