Create fastpath for LRegExp

RESOLVED FIXED in Firefox 56

Status

()

P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: h4writer, Assigned: jandem)

Tracking

(Blocks: 2 bugs, {perf})

unspecified
mozilla56
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
Posted patch WIP (obsolete) — Splinter Review
Was too easy, so still need to look to see what I forgot. Also didn't run tests yet.
(Reporter)

Updated

4 years ago
Blocks: 806646
(Reporter)

Updated

3 years ago
Priority: -- → P3
(Reporter)

Updated

3 years ago
Blocks: 1307062
(Reporter)

Updated

3 years ago
Keywords: perf
(Assignee)

Updated

2 years ago
Blocks: 1368419
(Assignee)

Comment 2

2 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

2 years ago
Posted 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+
(Assignee)

Comment 5

2 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

2 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)

Updated

2 years ago
Depends on: 1378736
(Assignee)

Comment 7

2 years ago
Posted 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+

Comment 9

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b0524a77e4
Optimize RegExpObject allocation in Ion. r=evilpie
(Assignee)

Comment 10

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76b0524a77e4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1379936
You need to log in before you can comment on or make changes to this bug.