- Notifications
You must be signed in to change notification settings - Fork5.2k
Improve linker performance by avoiding IndexOf#90721
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedAug 17, 2023
Tagging subscribers to 'linkable-framework':@eerhardt,@vitek-karas,@LakshanF,@sbomer,@joperezr,@marek-safar Issue DetailsFixes#90717 @sbomer@vitek-karas These edits are done online as I currently don't have a local fork of runtime. In case this needed more changes or refactoring, would you be able to take it over?
|
| for(inti=firstInstr;i<=lastInstr;i++){ | ||
| if(instructions[i]==target){ | ||
| returntrue; | ||
| } | ||
| } |
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.
This loop will have maximum of three iterations. This is a lot better thanIndexOf which will search through the wholeinstructions collection which can have thousands of 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.
Should we even bother with the lookup then?
Can we measure this - if the lookup doesn't bring interesting perf wins I would vote for keeping it simple and just do the linear search over the known possible range (that should be really fast).
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.
@vitek-karas I'm not sure about the logic and wanted to avoid potentially changing the meaning of the code, e.g, I have no idea if the dictionary lookup could yield a different result than the loop. So, I wanted to make a change that is clear it doesn't change the code semantics, and at the same time is more performant.
Regarding measuring this, I currently don't have the dotnet/runtime cloned locally to try something (the changes in this PR are done through GitHub UI actually). But I think the performance trace is quite clear this lookup is problematic, and it also the change should hopefully be making sense as a performance improvement. Yet, I understand that measuring would still be a good idea to at least know how much improvement this is. But unfortunately, I'm not able to do it right now. If necessary, would someone from the team be able to do it?
Youssef1313 commentedAug 17, 2023
CI failure is looking unrelated to the change here: |
sbomer left a comment
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.
Change LGTM, thanks!
lewing commentedAug 18, 2023
failures are known (and resoved) |
Fixes#90717
@sbomer@vitek-karas These edits are done online as I currently don't have a local fork of runtime. In case this needed more changes or refactoring, would you be able to take it over?