Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
lewing merged 2 commits intodotnet:mainfromYoussef1313:linker-perf
Aug 18, 2023

Conversation

@Youssef1313
Copy link
Member

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?

jeromelaban reacted with rocket emoji
@ghostghost added linkable-frameworkIssues associated with delivering a linker friendly framework area-Tools-ILLink.NET linker development as well as trimming analyzers community-contributionIndicates that the PR has been added by a community member labelsAug 17, 2023
@ghost
Copy link

Tagging subscribers to 'linkable-framework':@eerhardt,@vitek-karas,@LakshanF,@sbomer,@joperezr,@marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

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?

Author:Youssef1313
Assignees:-
Labels:

linkable-framework

Milestone:-

Comment on lines +108 to +112
for(inti=firstInstr;i<=lastInstr;i++){
if(instructions[i]==target){
returntrue;
}
}
Copy link
MemberAuthor

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.

Copy link
Member

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).

Copy link
MemberAuthor

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
Copy link
MemberAuthor

CI failure is looking unrelated to the change here:

    Wasi.Build.Tests.WasiTemplateTests.ConsoleBuildAndRunForDifferentOutputPaths(config: "Debug", appendRID: True, useArtifacts: False) [FAIL]      Runtime pack path doesn't match.      Expected: '/root/helix/work/workitem/e/dotnet-latest/packs/Microsoft.NETCore.App.Runtime.Mono.wasi-wasm/8.0.0-ci'      Actual:   '/root/helix/work/workitem/e/dotnet-latest/packs/Microsoft.NETCore.App.Runtime.Mono.wasi-wasm/8.0.0-rc.1.23414.4'      Stack Trace:        /_/src/mono/wasi/Wasi.Build.Tests/BuildTestBase.cs(346,0): at Wasm.Build.Tests.BuildTestBase.AssertRuntimePackPath(String buildOutput, String targetFramework)        /_/src/mono/wasi/Wasi.Build.Tests/BuildTestBase.cs(251,0): at Wasm.Build.Tests.BuildTestBase.BuildProject(BuildArgs buildArgs, String id, BuildProjectOptions options)        /_/src/mono/wasi/Wasi.Build.Tests/WasiTemplateTests.cs(165,0): at Wasi.Build.Tests.WasiTemplateTests.ConsoleBuildAndRunForDifferentOutputPaths(String config, Boolean appendRID, Boolean useArtifacts)           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)           at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)

Copy link
Member

@sbomersbomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Change LGTM, thanks!

Youssef1313 and jeromelaban reacted with heart emoji
@lewing
Copy link
Member

failures are known (and resoved)

@lewinglewing merged commit54fbd47 intodotnet:mainAug 18, 2023
@Youssef1313Youssef1313 deleted the linker-perf branchAugust 19, 2023 03:02
@ghostghost locked asresolvedand limited conversation to collaboratorsSep 18, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@vitek-karasvitek-karasvitek-karas left review comments

@sbomersbomersbomer approved these changes

@marek-safarmarek-safarAwaiting requested review from marek-safarmarek-safar is a code owner

Assignees

No one assigned

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzerscommunity-contributionIndicates that the PR has been added by a community memberlinkable-frameworkIssues associated with delivering a linker friendly framework

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UnrechableBlocksOptimizer has bad performance

4 participants

@Youssef1313@lewing@sbomer@vitek-karas

[8]ページ先頭

©2009-2025 Movatter.jp