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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file)

Attached patch Patch (deleted) — 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)
(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 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
(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.
Blocks: 1417038
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: