-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Speed up ILEmitTrie jump table using Binary Search Tree #52812
Comments
I know what the ILEmitTrie does but I've never touched it. From looking at the file history, no one has made significant changes to it since @rynowak wrote it years ago. Snaps for Ryan. If the tests all pass and perf is good then we're halfway there. I'd like to get someone to double check the vertorized path. I don't have enough expertise to review code and offer an opinion. What should think about:
|
@andrewjsaid Go ahead and create a PR. We should have discussion in the PR. |
I can probably provide some degree of feedback on topics like this in the PR. Unfortunately it's all from memory because I didn't work on this code much after the original version 😓 The good news is that I think the stakes probably pretty low. Most of the decisions like the switch between emit and dictionary were made empirically based on the benchmarks. So if the benchmarks change then the breakpoints should change too. Binary search wasn't something I thought of while building the original version. Compared to the previous version of routing all of this work was in a different galaxy perf-wise 🪐 . Most of my time was spent worrying about overhead in the
I think @JamesNK raises a good point about code size. Changing the complexity to |
The proposal here is that we can improve
ILEmitTrie
by using binary search instead of the linear search we currently do for both finding the length as well as the vectorized search.This improves the MicroBenchmarks as follows:
Implementation in andrewjsaid#1. The PR template implied I had to first create an issue before the PR so I did so. Please let me know if design makes sense I can then point the PR to here for detailed code review.
In the linked PR you can see the changes are:
Before
After
The text was updated successfully, but these errors were encountered: