Closed
Bug 1235046
Opened 8 years ago
Closed 8 years ago
Make code poisoning in JitCode::finalize faster with W^X
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file)
14.11 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
We poison JIT code (in JitCode::finalize) in all builds. With W^X enabled that can be pretty slow: we often mprotect the exact same pages while we finalize different JitCodes. The attached patch optimizes this: JitCode::finalize adds a JitPoisonRange entry to a Vector stored in FreeOp (similar to the freeLater Vector there). Then in ~FreeOp, we pass this Vector to ExecutableAllocator::poisonCode, where we can do the poisoning much more efficiently by ensuring each ExecutablePool is made writable only once. On a pretty worst-case benchmark, this reduces the time spent poisoning from 57 ms to 2 ms or so.
Attachment #8701814 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 1•8 years ago
|
||
Comment on attachment 8701814 [details] [diff] [review] Patch Review of attachment 8701814 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Runtime.h @@ +1600,5 @@ > +inline bool > +FreeOp::appendJitPoisonRange(const jit::JitPoisonRange& range) > +{ > + // FreeOps other than the defaultFreeOp() are constructed on the stack, > + // and won't hold onto the pointers to free indefinitely. Ugh, copy/paste typo. s/pointers to free/ranges to poison/
Comment 2•8 years ago
|
||
Comment on attachment 8701814 [details] [diff] [review] Patch Review of attachment 8701814 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/ExecutableAllocator.cpp @@ +370,5 @@ > + > + // Don't race with reprotectAll called from the signal handler. > + JitRuntime::AutoPreventBackedgePatching apbp(rt); > + > + for (size_t i = 0; i < ranges.length(); i++) { Maybe have an #ifdef DEBUG loop before this one that asserts the mark bits are clear on all the pools? ::: js/src/jit/Ion.cpp @@ +873,5 @@ > + MOZ_ASSERT(pool_); > + > + // With W^X JIT code, reprotecting memory for each JitCode instance is > + // slow, so we just record the ranges and poison them later all at once. > + if (fop->appendJitPoisonRange(JitPoisonRange(pool_, code_, bufferSize_))) A note about why it's ok to ignore append failure would be good here.
Attachment #8701814 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #2) > Maybe have an #ifdef DEBUG loop before this one that asserts the mark bits > are clear on all the pools? Good idea, done.
Comment 5•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd9728713ebb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•