Skip to content

[wasm] optimize out 'is this method MulticastDelegate.Invoke' checks in aot->interp transitions #80181

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
kg merged 2 commits into from
Jan 10, 2023
Merged

[wasm] optimize out 'is this method MulticastDelegate.Invoke' checks in aot->interp transitions #80181

kg merged 2 commits into from
Jan 10, 2023

Conversation

@kg
Copy link
Member

@kg kg commented Jan 4, 2023 •
edited
Loading

Right now interp_entry and jiterpreter's equivalent need to check for every call whether the call target is MulticastDelegate.Invoke. While this sounds inexpensive, in browser-bench's JSON suite that check by itself was ~0.5% of cpu time according to Chrome's profiler. This PR updates InterpMethod creation to store a flag for this so we can just check the flag during interp_entry to avoid the wasted work.

@kg kg added the arch-wasm WebAssembly architecture label Jan 4, 2023
@ghost ghost assigned kg Jan 4, 2023
@ghost
Copy link

ghost commented Jan 4, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Right now interp_entry and jiterpreter's equivalent need to check for every call whether the call target is MulticastDelegate.Invoke. While this sounds inexpensive, in browser-bench's JSON suite that check by itself was ~0.5% of cpu time according to Chrome's profiler. We can know pretty trivially that if a method isn't called "Invoke" it couldn't possibly become MulticastDelegate.Invoke later on, which allows us to skip the check in the jiterpreter's entry routine for those call targets.

Maybe we should do this for the actual interpreter during codegen too?

Author: kg
Assignees: -
Labels:

arch-wasm
Milestone: -

@kg kg marked this pull request as ready for review January 4, 2023 17:22
@kg
Copy link
Member Author

kg commented Jan 4, 2023

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 4, 2023

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@kg
Copy link
Member Author

kg commented Jan 4, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 4, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@lewing
Copy link
Member

lewing commented Jan 4, 2023

/azp list

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 4, 2023

CI/CD Pipelines for this repository:

@radical
Copy link
Member

radical commented Jan 4, 2023

/azp run runtime

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 4, 2023

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member

radical commented Jan 4, 2023

Fix for the STJ failures was just merged - #80186 .

@lewing lewing closed this Jan 5, 2023
@lewing lewing reopened this Jan 5, 2023
@kg kg force-pushed the jiterpreter-can-be-invoke branch from 412657e to affc9ea Compare January 5, 2023 18:43
@kg
Copy link
Member Author

kg commented Jan 5, 2023

The S.T.J AOT failures looked relevant, but I can't reproduce them so I'm bumping to latest upstream to re-run.

@lewing
Copy link
Member

lewing commented Jan 5, 2023

the wbt failures look like #80241

lewing
lewing reviewed Jan 5, 2023
View reviewed changes
@build-analysis build-analysis bot mentioned this pull request Jan 6, 2023
Closed
@kg kg force-pushed the jiterpreter-can-be-invoke branch from affc9ea to d77b943 Compare January 6, 2023 21:45
@kg kg requested review from lambdageek and thaystg as code owners January 6, 2023 21:45
@kg kg changed the title [wasm] optimize out 'is this method MulticastDelegate.Invoke' checks in jiterpreter invokes [wasm] optimize out 'is this method MulticastDelegate.Invoke' checks in aot->interp transitions Jan 6, 2023
@kg kg force-pushed the jiterpreter-can-be-invoke branch 2 times, most recently from e3948f8 to 5a2c50f Compare January 6, 2023 22:20
vargaz
vargaz reviewed Jan 7, 2023
View reviewed changes
@kg
When creating an InterpMethod, identify whether it is MulticastDelega…
55951a7 
…te.Invoke and store that in a flag to optimize interp_entry

Always inline some jiterpreter APIs
@kg kg force-pushed the jiterpreter-can-be-invoke branch from 5a2c50f to 55951a7 Compare January 7, 2023 01:42
lewing
lewing reviewed Jan 9, 2023
View reviewed changes
@kg kg merged commit 916c691 into dotnet:main Jan 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lewing lewing lewing left review comments

@pavelsavara pavelsavara Awaiting requested review from pavelsavara pavelsavara is a code owner

@BrzVlad BrzVlad Awaiting requested review from BrzVlad BrzVlad is a code owner

@lambdageek lambdageek Awaiting requested review from lambdageek

@thaystg thaystg Awaiting requested review from thaystg

+1 more reviewer

@vargaz vargaz vargaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kg kg

Labels

arch-wasm WebAssembly architecture area-Codegen-Interpreter-mono

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kg @lewing @radical @vargaz @BrzVlad