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

[mono] Enable MBR by default if interpreter is enabled #46842

Closed

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Jan 12, 2021

Enable Mono MBR support by default.
Goal is to check that there are no perf regressions for non-MBR scenarios

@lambdageek lambdageek added NO-REVIEW Experimental/testing PR, do NOT review it NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-VM-meta-mono labels Jan 12, 2021
@ghost
Copy link

ghost commented Jan 12, 2021

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

Issue Details

For performance testing

Author: lambdageek
Assignees: -
Labels:

* NO MERGE *, NO REVIEW, area-Build-mono, area-VM-meta-mono

Milestone: -

@lambdageek lambdageek changed the base branch from master to alklig/enable-metadata-update January 12, 2021 03:05
@lambdageek lambdageek force-pushed the alklig/enable-metadata-update branch from 8027936 to f0bdc6b Compare January 12, 2021 03:07
@lambdageek lambdageek changed the base branch from alklig/enable-metadata-update to master January 12, 2021 03:08
@lambdageek lambdageek closed this Jan 12, 2021
@lambdageek lambdageek reopened this Jan 12, 2021
@lambdageek
Copy link
Member Author

lambdageek commented Jan 12, 2021

@naricc assuming the tests here look ok, would it be possible to do a perf comparison of alklig/enable-metadata-update branch in dotnet/runtime vs the normal runtime. primarily interested in interp on desktop and wasm. But for sanity-checking also normal JIT numbers are interesting - just to make sure I didn't put update checking on some hotpath.

@naricc
Copy link
Contributor

naricc commented Jan 12, 2021

@lambdageek Yes; I should be able to do performance runs tomorrow. The runs take on the order of hours, so probably we will have the results tomorrow afternoon. The way it is setup it always runs everything in the pipeline, so we will get results for JIT, desktop interpreter, and WASM.

@lambdageek lambdageek force-pushed the alklig/enable-metadata-update branch from f0bdc6b to 2f8c25a Compare January 12, 2021 03:48
@naricc naricc force-pushed the alklig/enable-metadata-update branch from 2f8c25a to b55b691 Compare January 15, 2021 16:22
@lambdageek
Copy link
Member Author

I've looked at profiles of interpreter results on Burgers2, Compare_Exchange(long) and System.Collections.Concurrent.Count<String> using Instruments on OSX. But in all cases, the actual timing of the runs was pretty inconsistent so I'm not convinced that the perf runs are highlighting an actual problem. (Wasm run was more definitive - I had turned off interpreter inlining on all wasm runs - it was hurting performance).

I was able to identify some differences in profiles, primarily due to profiler choices around mono_metadata_decode_row and mono_metadata_decode_row_col. I added #47716, but it doesn't not alter the run times in any meaningful way. (it does nudge the xcode clang inliner to the same choices as before, so the profile stack traces look more similar, and I think it's better to logically separate the slow path with an explicit state change.)

My overall conclusion is that the microbenchmark runs did not highlight actionable issues in turning on the metadata update code by default.

The other PRs that came out of this perf investigation are:

@lambdageek
Copy link
Member Author

Closing this PR. it was only for the perf investigation

@lambdageek lambdageek closed this Feb 2, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2021
@lambdageek lambdageek deleted the alklig/enable-metadata-update branch March 7, 2021 21:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-meta-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants