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

Remove use of handles in interp_exec_method and instead use a reserved stack slot #80197

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

kg
Copy link
Contributor

@kg kg commented Jan 4, 2023

Right now every time we enter the interpreter we waste some CPU time allocating a handle, even though the only remaining use of the handle is in the BOX opcodes. This PR instead allocates some space at the top of the stack for a temporary GC-visible object pointer, and then uses that instead of a handle. For my AOT<->interp transition stress test a ~30sec profile run was spending over a second allocating handles and this optimizes that out entirely.

@kg kg added the arch-wasm WebAssembly architecture label Jan 4, 2023
@kg kg requested review from BrzVlad and vargaz as code owners January 4, 2023 21:12
@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 every time we enter the interpreter we waste some CPU time allocating a handle, even though the only remaining use of the handle is in the BOX opcodes. This PR instead allocates some space at the top of the stack for a temporary GC-visible object pointer, and then uses that instead of a handle. For my AOT<->interp transition stress test a ~30sec profile run was spending over a second allocating handles and this optimizes that out entirely.

Author: kg
Assignees: -
Labels:

arch-wasm

Milestone: -

@vargaz
Copy link
Contributor

vargaz commented Jan 4, 2023

Is the overhead from HANDLE_FUNCTION_ENTER ()/EXIT () ? Allocating a handle shouldn't be that slow.

@kg
Copy link
Contributor Author

kg commented Jan 4, 2023

Is the overhead from HANDLE_FUNCTION_ENTER ()/EXIT () ? Allocating a handle shouldn't be that slow.

I don't know specifically where the overhead comes from, but the handle_new time in profiles was really significant. It's in a microbenchmark that does tons of transitions, of course, but they show up for json in browser-bench AOT'd as well. It seemed like deferring the handle creation optimized it out in my initial tests, but removing the handles entirely makes sure it's gone.

It might be a WASM-specific problem and maybe handles are faster on native?

@vargaz
Copy link
Contributor

vargaz commented Jan 5, 2023

Looks ok, hopefully it won't break anything.

@vargaz
Copy link
Contributor

vargaz commented Jan 5, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@srxqds
Copy link
Contributor

srxqds commented Jan 5, 2023

so if it works, hope can backport to release/7.0

@kg kg merged commit 05f0b1a into dotnet:main Jan 5, 2023
@@ -406,7 +406,8 @@ get_context (void)
context->stack_start = (guchar*)mono_valloc (0, INTERP_STACK_SIZE, MONO_MMAP_READ | MONO_MMAP_WRITE, MONO_MEM_ACCOUNT_INTERP_STACK);
context->stack_end = context->stack_start + INTERP_STACK_SIZE - INTERP_REDZONE_SIZE;
context->stack_real_end = context->stack_start + INTERP_STACK_SIZE;
context->stack_pointer = context->stack_start;
/* We reserve an object pointer slot at the top of the interp stack to make temp objects visible to GC */
context->stack_pointer = context->stack_start + sizeof(MonoObject*);
Copy link
Member

Choose a reason for hiding this comment

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

We always keep the stack pointer and all interp vars aligned to MINT_STACK_SLOT_SIZE, which is 8 bytes. On 32 bit this breaks alignment. This doesn't appear to be a critical problem, but it would be best to maintain the default alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix it, thanks.

kg added a commit that referenced this pull request Jan 6, 2023
#80197 accidentally broke interp stack alignment on 32-bit platforms due to pointers being 4 bytes there.
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2023
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.

4 participants