Closed
Bug 1416572
Opened 7 years ago
Closed 7 years ago
Use a single JitCode instance for all VMFunction wrappers
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file)
(deleted),
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
We create VM wrappers for all VMFunctions when we initialize a JitRuntime. There are currently 263 of these wrappers and for each of them we have to initialize a MacroAssembler, allocate a JitCode GC thing, mprotect the code, etc. This can be pretty slow, especially on mobile.
Once upon a time, we used to allocate VMFunction wrappers lazily and so it was necessary for each of them to have its own JitCode. Now that wrappers are never collected and all have the same lifetime, we can just use a single MacroAssembler/JitCode and for each wrapper save the offset into this buffer.
On OS X this improves the time to generate all VMFunction wrappers from 1.0-1.5 ms to 0.15 ms or so.
There's more we can improve after this:
* We can move the other JitRuntime trampolines into this shared JitCode instance as well.
* After that we could consider getting rid of this shared JitCode instance and just share trampoline/wrapper code across all runtimes. That's not too easy because trampolines currently bake in JSRuntime* pointers, but we could pass the runtime or context as argument.
* Exit frames currently store the wrapper's JitCode so the GC can mark it. This used to be necessary but since wrappers are never collected nowadays, we could get rid of this field (and then the VMFunction* pointer could be used to store the ExitFrameToken).
Attachment #8927666 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 1•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #0)
> This can be pretty slow, especially on mobile.
On mobile this was extra slow because of the icache flushing we used to do for each wrapper we allocate. We will now flush the whole range at once.
Comment 2•7 years ago
|
||
Comment on attachment 8927666 [details] [diff] [review]
Patch
Review of attachment 8927666 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/Ion.cpp
@@ +748,5 @@
>
> + uint32_t offset = p->value();
> + MOZ_ASSERT(offset < functionWrapperCode_->instructionsSize());
> +
> + return functionWrapperCode_->raw() + offset;
follow-up: It is weird to see non-checked pointers. I suggest we change the return type to JitCodeInnerPtr, with something like:
using JitCodeInnerPtr = NotNull<uint8_t*>;
I guess doing that for JitCode::raw() might cause us to propagate this new type to the MacroAssembler, for the best.
::: js/src/jit/x86-shared/MacroAssembler-x86-shared.h
@@ +540,5 @@
> }
> void jump(JitCode* code) {
> jmp(code);
> }
> + void jump(ImmPtr code) {
nit: Having a JitCodeInnerPtr should help remove this use case.
Attachment #8927666 -
Flags: review?(nicolas.b.pierron) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8d8a1764cba
Use a single JitCode instance for all VMFunction wrappers. r=nbp
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > + void jump(ImmPtr code) {
>
> nit: Having a JitCodeInnerPtr should help remove this use case.
I'm not sure if it's worth the trouble - having a jump(ImmPtr) method doesn't seem too unreasonable to me.
Comment 5•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•