Skip to content

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

Merged
vargaz merged 1 commit into
dotnet:mainfrom
vargaz:gshared-new
Apr 26, 2023
Merged

[mono][jit] Use an mrgctx for all gshared methods on WASM.#82981
vargaz merged 1 commit into
dotnet:mainfrom
vargaz:gshared-new

Conversation

@vargaz

@vargaz vargaz commented Mar 4, 2023

Copy link
Copy Markdown
Contributor

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

vargaz commented Mar 4, 2023

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz

vargaz commented Mar 4, 2023

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz

vargaz commented Mar 4, 2023

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz

vargaz commented Mar 4, 2023

Copy link
Copy Markdown
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines

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

vargaz commented Mar 8, 2023

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@vargaz

vargaz commented Mar 8, 2023

Copy link
Copy Markdown
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz

vargaz commented Mar 11, 2023

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@vargaz

vargaz commented Mar 25, 2023

Copy link
Copy Markdown
Contributor Author

/azp run runtime-wasm

@azure-pipelines

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

kotlarmilos commented Mar 29, 2023

Copy link
Copy Markdown
Member

/azp run runtime-ioslike

@azure-pipelines

Copy link
Copy Markdown
No pipelines are associated with this pull request.

Comment thread src/mono/mono/utils/options-def.h Outdated

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 it include HOST_IOS as well?

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@kotlarmilos

Copy link
Copy Markdown
Member

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

@vargaz

vargaz commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

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

@vargaz

vargaz commented Mar 29, 2023

Copy link
Copy Markdown
Contributor Author

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

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

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

@vargaz

vargaz commented Apr 24, 2023

Copy link
Copy Markdown
Contributor Author

It could be merged.

@kotlarmilos

Copy link
Copy Markdown
Member

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

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

vargaz commented Apr 26, 2023

Copy link
Copy Markdown
Contributor Author

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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants