-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove fold() closures from FieldSet._hashCode
.
#554
Conversation
The fold() calls come with significant runtime cost, and make it more difficult to hoist accesses to commonly used variables. Just rewriting this pattern shows a significant performance improvement for both JS and VM benchmarks. Baseline ``` JS HashCode(RunTime): 9833.333333333334 us. HashCode(RunTime): 9891.625615763547 us. HashCode(RunTime): 9607.655502392345 us. HashCode(RunTime): 9661.835748792271 us. HashCode(RunTime): 9765.853658536585 us. VM HashCode(RunTime): 4527.384615384615 us. HashCode(RunTime): 4534.151583710407 us. HashCode(RunTime): 4546.556818181818 us. HashCode(RunTime): 4490.65470852017 ``` Results ``` JS HashCode(RunTime): 8004 us. HashCode(RunTime): 7980.0796812749 us. HashCode(RunTime): 7976.095617529881 us. HashCode(RunTime): 7824.21875 us. HashCode(RunTime): 7847.058823529412 us. VM HashCode(RunTime): 2474.5451174289246 us. HashCode(RunTime): 2533.7037974683544 us. HashCode(RunTime): 2532.4556962025317 us. HashCode(RunTime): 2420.072551390568 us. HashCode(RunTime): 2521.3198992443326 ```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Do you think the existing tests are adequate to pick up a bug if you had introduced one?
(I don't think you did, but that is always a concern.)
}); | ||
// Hash with non-extension fields. | ||
final values = _values; | ||
for (final fi in _infosSortedByTag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_hashObjects
, called at line 671, also uses fold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a big difference between these two patterns. The function passed to fold()
in _hashObjects
doesn't capture any local variables. I tested modifying that one as well and it was a regression in both JS and VM cases.
I wonder, now that we're looking into the hashing, if _HashUtils could be replaced by |
I suspect performance would be worse. There is no way way I recommend this change goes ahead as-is. It is a nice performance boost for real apps. Sigurd, are you an 'owner' that can approve this change? |
I think protobuf is a bit "between owners". My main worry is the syncing to our internal repo. But besides that I approve - feel free to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I also run the benchmarks locally again and got similar improvements.
One of the CI checks was cancelled but for some reason I can't restart it now. Merging.
Ported to internal in cl/449694660. |
The fold() calls come with significant runtime cost, and make it more
difficult to hoist accesses to commonly used variables. Just rewriting
this pattern shows a significant performance improvement for both JS and
VM benchmarks.
Baseline
Results