Skip to content

[mono][jit] Use an mrgctx for all gshared methods on WASM. #82981

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 26, 2023
Merged

[mono][jit] Use an mrgctx for all gshared methods on WASM. #82981

merged 1 commit into from
Apr 26, 2023

Conversation

@vargaz
Copy link
Contributor

@vargaz vargaz commented Mar 4, 2023

No description provided.

@ghost ghost added the area-Codegen-JIT-mono label Mar 4, 2023
@vargaz vargaz added NO-REVIEW Experimental/testing PR, do NOT review it and removed area-Codegen-JIT-mono labels Mar 4, 2023
@ghost ghost assigned vargaz Mar 4, 2023
@vargaz
Copy link
Contributor Author

vargaz commented Mar 4, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 4, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Mar 4, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 4, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Mar 4, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 4, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Mar 4, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 4, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz vargaz force-pushed the gshared-new branch 2 times, most recently from 4b26b61 to fe193bd Compare March 8, 2023 19:48
@vargaz
Copy link
Contributor Author

vargaz commented Mar 8, 2023

/azp run runtime-wasm

@vargaz
Copy link
Contributor Author

vargaz commented Mar 8, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 8, 2023

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

azure-pipelines bot commented Mar 8, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Mar 8, 2023
Closed
@vargaz vargaz force-pushed the gshared-new branch 2 times, most recently from c1f47c8 to 8203760 Compare March 8, 2023 22:37
@vargaz vargaz mentioned this pull request Mar 9, 2023
Closed
This was referenced Mar 9, 2023
Closed
Closed
@vargaz
Copy link
Contributor Author

vargaz commented Mar 11, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 11, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor Author

vargaz commented Mar 25, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 25, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz vargaz changed the title [mono][jit] Use an mrgctx for all gshared methods. [mono][jit] Use an mrgctx for all gshared methods on WASM. Mar 28, 2023
@build-analysis build-analysis bot mentioned this pull request Mar 28, 2023
Closed
@vargaz vargaz marked this pull request as ready for review March 28, 2023 14:49
@vargaz vargaz force-pushed the gshared-new branch 2 times, most recently from 40f1f7e to 13b04b0 Compare March 28, 2023 15:06
@kotlarmilos
Copy link
Member

kotlarmilos commented Mar 29, 2023 •
edited
Loading

/azp run runtime-ioslike

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 29, 2023

No pipelines are associated with this pull request.

kotlarmilos
kotlarmilos reviewed Mar 29, 2023
View reviewed changes
src/mono/mono/utils/options-def.h Outdated
Copy link
Member

@kotlarmilos kotlarmilos Mar 29, 2023

Choose a reason for hiding this comment

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

Should it include HOST_IOS as well?

Copy link
Contributor Author

@vargaz vargaz Apr 2, 2023

Choose a reason for hiding this comment

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

Its better to start with one platform to see how it goes.

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 29, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos
Copy link
Member

kotlarmilos commented Mar 29, 2023

As it might have performance disadvantages, is there a way to test it locally before merging?

@vargaz
Copy link
Contributor Author

vargaz commented Mar 29, 2023

The benchmarking suite can be run on wasm locally, not sure about ios.

@vargaz
Copy link
Contributor Author

vargaz commented Mar 29, 2023

Also, it could be merged and later reverted if the perf impact is significant.

@vargaz
[mono][jit] Add an option to use an mrgctx for all gshared methods.
f796170 
Enable it by default on WASM.

In this mode, all gshared methods get an mrgctx, which means they can access
their data using a simple load from the mrgctx instead of having to call
a rgctx fetch trampoline.

Upsides:
- much simpler.
- faster access to gshared data
- smaller code and data size in the AOT case
- if enabled by default on all platforms, large amount of gshared
  code can be removed

Downsides:
- the methods have to initialize their mrgctx in their prolog
- on non-wasm platforms, indirect calls to gshared methods
  (like virtual calls) will need to use rgctx trampolines more often
  to pass the mrgctx.
@kotlarmilos
Copy link
Member

kotlarmilos commented Apr 24, 2023

@vargaz is it ready to be merged after the Preview 4 snap? Is there anything else missing?

@vargaz
Copy link
Contributor Author

vargaz commented Apr 24, 2023

It could be merged.

@kotlarmilos
Copy link
Member

kotlarmilos commented Apr 24, 2023

It could be merged.

Good. I will approve it after the Preview 4 snap.

@kotlarmilos kotlarmilos self-requested a review April 26, 2023 08:24
kotlarmilos
kotlarmilos approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! Hopefully, we can reduce the size with reasonable speed tradeoff.

P.S. I would leave this to @lambdageek for final approval.

/cc: @SamMonoRT

@vargaz vargaz merged commit 184d17d into dotnet:main Apr 26, 2023
@vargaz vargaz deleted the gshared-new branch April 26, 2023 15:01
@vargaz
Copy link
Contributor Author

vargaz commented Apr 26, 2023

Will see what changes this will do to benchmarks.

@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kotlarmilos kotlarmilos kotlarmilos approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

Assignees

@vargaz vargaz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vargaz @kotlarmilos