Skip to content

Improve linker performance by avoiding IndexOf - #90721

Merged
lewing merged 2 commits into
dotnet:mainfrom
Youssef1313:linker-perf
Aug 18, 2023
Merged

Improve linker performance by avoiding IndexOf#90721
lewing merged 2 commits into
dotnet:mainfrom
Youssef1313:linker-perf

Conversation

@Youssef1313

Copy link
Copy Markdown
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?

@ghost ghost added linkable-framework Issues associated with delivering a linker friendly framework area-Tools-ILLink .NET linker development as well as trimming analyzers community-contribution Indicates that the PR has been added by a community member labels Aug 17, 2023
@ghost

ghost commented Aug 17, 2023

Copy link
Copy Markdown

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 (int i = firstInstr; i <= lastInstr; i++) {
if (instructions[i] == target) {
return true;
}
}

Copy link
Copy Markdown
Member Author

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 than IndexOf which will search through the whole instructions collection which can have thousands of elements.

Copy link
Copy Markdown
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
Copy Markdown
Member Author

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
Copy Markdown
Member Author

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)

@sbomer sbomer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change LGTM, thanks!

@lewing

lewing commented Aug 18, 2023

Copy link
Copy Markdown
Member

failures are known (and resoved)

@lewing
lewing merged commit 54fbd47 into dotnet:main Aug 18, 2023
@Youssef1313
Youssef1313 deleted the linker-perf branch August 19, 2023 03:02
@ghost ghost locked as resolved and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers community-contribution Indicates that the PR has been added by a community member linkable-framework Issues associated with delivering a linker friendly framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UnrechableBlocksOptimizer has bad performance

4 participants