-
Notifications
You must be signed in to change notification settings - Fork 105
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
Improve performance of variable resolver #672
Conversation
I'm going to tweak it a little more before I land it, stay tuned. |
Switch to a hash table when the number of variables grows beyond a threshold. Speeds up the test case from the linked issue by about 70%. Fixes: quickjs-ng#456
00d3772
to
129932e
Compare
for (p = b; p < (char *)b + n; p++) | ||
h = 33*h + *p; | ||
h += h >> 5; |
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.
Switching the hash function resulted in a whopping 15% fewer hash collisions. Pretty big deal.
Catastrophic collisions are still going to be an issue once the table grows to a few million elements but I hope and assume that's never going to happen in the real world. Even that monster from the linked issue has < 20,000 elements.
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.
Nice! Random question: we seem to have a few htables around now, do you see an opportunity to DRY some out? Not in this PR of course.
This would be great. Maybe even pulling in something external? https://github.com/JacksonAllan/Verstable :D |
Dunno. They're all just different enough from each other that it's not a slam dunk DRY job. I did notice that, compared to perl hash, Swapping the hash function didn't make a measurable difference in my quick testing though, likely because the shape and IC hash tables just don't get that big. |
Switch to a hash table when the number of variables grows beyond a threshold.
Speeds up the test case from the linked issue by about 70%.
Fixes: #456