Closed
Bug 1115355
Opened 10 years ago
Closed 7 years ago
Create fastpath for LRegExp
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: h4writer, Assigned: jandem)
References
(Blocks 2 open bugs)
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
We do a vmcall for LRegExp, we can inline the allocation. This made a 28% difference on v8. So I was very hopefull. Though doing something similar on ionmonkey only gained us 6%. A bit dissappointing.
Reporter | ||
Comment 1•10 years ago
|
||
Was too easy, so still need to look to see what I forgot. Also didn't run tests yet.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•7 years ago
|
||
I rebased this patch and on top of bug 1368461 it's a pretty big win on the six-speed tests. Probably about as fast as V8/JSC on these tests or a bit faster.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 3•7 years ago
|
||
This is the patch from Hannes rebased to tip. Pretty big win on the six-speed and Octane regexp tests.
Assignee: nobody → jdemooij
Attachment #8541245 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8883609 -
Flags: review?(evilpies)
Comment 4•7 years ago
|
||
Comment on attachment 8883609 [details] [diff] [review]
Patch
Review of attachment 8883609 [details] [diff] [review]:
-----------------------------------------------------------------
Nice! Awesome to get this fixed. Can we assert MOZ_ASSERT(lir->mir()->source()->hasShared()), which seems to me like the only thing that isn't straight up copying slots or the private pointer.
Attachment #8883609 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #4)
> Can we assert
> MOZ_ASSERT(lir->mir()->source()->hasShared()), which seems to me like the
> only thing that isn't straight up copying slots or the private pointer.
Good point, I haven't tried it yet but I don't think that will hold, because (1) The RegExpShared is initialized on first clone, and (2) The RegExpShared can be discarded at any time on GC [1].
The only downside to cloning without forcing a RegExpShared is that we will have to look up our RegExpShared for each clone we execute, and Ion's fast paths for regex execution may not work.
So maybe we should just check in JIT code whether the template object has a RegExpShared and take the slow path if it hasn't. I'll try that...
[1] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/js/src/vm/RegExpObject.cpp#181-184
Assignee | ||
Comment 6•7 years ago
|
||
I just realized there is a correctness issue because the private slot stores RegExpShared* and that's:
(1) A GC thing that MacroAssembler::initGCThing needs to copy as an ImmGCPtr since it's movable.
(2) A ReadBarriered pointer (because it can be null'd on GC) which means we have to trigger a read barrier. jonco and I discussed this on IRC and we think we may get away with not nulling it on GC since we can discard regex JitCode anyway. Then we can get rid of the read barrier and improve regex perf a bit and also make this bug much easier to fix.
Assignee | ||
Comment 7•7 years ago
|
||
With the dependent bugs fixed, this isn't too hard. We just have to use ImmGCPtr for the RegExpShared* and we always use a VM call if the template object does not have a RegExpShared* (this means the op never executed before).
Attachment #8883609 -
Attachment is obsolete: true
Attachment #8884512 -
Flags: review?(evilpies)
Comment 8•7 years ago
|
||
Comment on attachment 8884512 [details] [diff] [review]
Patch v2
Review of attachment 8884512 [details] [diff] [review]:
-----------------------------------------------------------------
Have you thought about eagerly making the regexp shared in IonBuilder? This way we could avoid always falling back to the VM call in rare cases.
Attachment #8884512 -
Flags: review?(evilpies) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b0524a77e4
Optimize RegExpObject allocation in Ion. r=evilpie
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #8)
> Have you thought about eagerly making the regexp shared in IonBuilder? This
> way we could avoid always falling back to the VM call in rare cases.
It's complicated because we don't have a JSContext* and can't GC in IonBuilder. We could use TLS but it's hackish. I think the current approach is fine: if a branch is never executed, in most cases it will either be pruned or it will bailout anyway because of type barriers.
Comment 11•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•