1115355 - Create fastpath for LRegExp

Status: RESOLVED FIXED

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

Description

[Hannes Verschore [:h4writer]]

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.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: WIP

Was too easy, so still need to look to see what I forgot. Also didn't run tests yet.

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

This is the patch from Hannes rebased to tip. Pretty big win on the six-speed and Octane regexp tests.

Comment 4

[Tom S [:evilpie]]

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.

Comment 5

[Jan de Mooij [:jandem]]

(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

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch v2

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

Comment 8

[Tom S [:evilpie]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b0524a77e4
Optimize RegExpObject allocation in Ion. r=evilpie

Comment 10

[Jan de Mooij [:jandem]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76b0524a77e4