Skip to content
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

[wasm] Disable interp pgo recording by default since it adds startup overhead #101566

Merged
merged 3 commits into from
Apr 27, 2024

Conversation

kg
Copy link
Member

@kg kg commented Apr 25, 2024

During startup for interpreted blazor we seem to spend a small but notable amount of time in here even if interp PGO isn't actually enabled. It's possible to set runtime options at startup, so anyone who wants to use interp PGO can just change this option in their app config, I think.

Open as draft for now because I think this will make a WBT fail.

Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

@kg kg marked this pull request as ready for review April 27, 2024 00:53
@kg kg merged commit e1928e4 into dotnet:main Apr 27, 2024
73 of 80 checks passed
Comment on lines 158 to +164
interpreterPgo: value,
interpreterPgoSaveDelay: autoSaveDelay
});
if (monoConfig.runtimeOptions)
monoConfig.runtimeOptions.push("--interp-pgo-recording");
else
monoConfig.runtimeOptions = ["--interp-pgo-recording"];
Copy link
Member

@maraf maraf Apr 27, 2024

Choose a reason for hiding this comment

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

The idea around here is that deep_merge_config solves this type of conditional assignement

Suggested change
interpreterPgo: value,
interpreterPgoSaveDelay: autoSaveDelay
});
if (monoConfig.runtimeOptions)
monoConfig.runtimeOptions.push("--interp-pgo-recording");
else
monoConfig.runtimeOptions = ["--interp-pgo-recording"];
interpreterPgo: value,
interpreterPgoSaveDelay: autoSaveDelay,
runtimeOptions: ["--interp-pgo-recording"]
});

Copy link
Member

@maraf maraf Apr 27, 2024

Choose a reason for hiding this comment

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

if (providedConfig.runtimeOptions !== undefined && providedConfig.runtimeOptions !== target.runtimeOptions) {
providedConfig.runtimeOptions = [...(target.runtimeOptions || []), ...(providedConfig.runtimeOptions || [])];

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, thanks. I'll make a follow-up PR to clean this up.

matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…overhead (dotnet#101566)

* Disable interp pgo recording by default since it adds startup overhead
* Make withRuntimeOptions not trample existing runtime options
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
…overhead (dotnet#101566)

* Disable interp pgo recording by default since it adds startup overhead
* Make withRuntimeOptions not trample existing runtime options
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants