Closed Bug 1115355 Opened 6 years ago Closed 3 years ago

Create fastpath for LRegExp

Categories

(Core :: JavaScript Engine: JIT, defect, P3)

x86_64
Linux
defect

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)

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.
Attached patch WIP (obsolete) — Splinter Review
Was too easy, so still need to look to see what I forgot. Also didn't run tests yet.
Blocks: 806646
Priority: -- → P3
Blocks: jsperf
Keywords: perf
Blocks: 1368419
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)
Attached patch Patch (obsolete) — Splinter Review
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 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+
(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
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.
Depends on: 1378736
Attached patch Patch v2Splinter Review
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 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
(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.
https://hg.mozilla.org/mozilla-central/rev/76b0524a77e4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.