1010441 - Keep RegExpShared around when preserving jitcode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently we always throw away almost all RegExpShared instances and regexp jitcode on GC.  It would be good to improve this, and allow these things to be traced as normal and kept around if we are also preserving Ion jitcode during the GC.  The attached patch makes these changes, and improves irregexp a bit as tested in bug 976446 comment 48.  It doesn't seem to improve yarr though and might even be making it slower which is kind of bizarre, and anyways landing this should wait until after the irregexp port lands.

This patch also improves various weird things about how RegExpShared instances are currently kept around when referenced by the C stack or captured via incremental barriers, reducing complexity and memory usage (both in RegExpShared instances themselves and in the RegExpCompartment tables).

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8422641 [details] [diff] [review]
patch

Review of attachment 8422641 [details] [diff] [review]:
-----------------------------------------------------------------

I just want to see the barrier changes again. I promise I'll get to it quickly when you re-post.

::: js/src/jscompartment.cpp
@@ +657,5 @@
>  #endif
>      JS_ASSERT(!debugScopes);
>      JS_ASSERT(!gcWeakMapList);
>      JS_ASSERT(enumerators->next() == enumerators);
> +    JS_ASSERT(regExps.empty());

I'm just curious: what makes this true? It seems like off-thread parsing would create regexp objects, and I don't see anything that moves them over afterwards.

::: js/src/vm/RegExpObject.cpp
@@ +795,5 @@
> +        // Sometimes RegExpShared instances are marked without the
> +        // compartment being subsequently cleared. This can happen if a GC is
> +        // restarted while in progress (i.e. performing a full GC in the
> +        // middle of an incremental GC) or if a RegExpShared referenced via the
> +        // stack is traced but is not in a zone being collected.

Thinking about this a little more, I don't think that stack references can cause us to do extra marking. References from Rooted<> just use MarkObjectRoot, and that checks isGCMarking() on the zone. So it's really only GC resets that can cause the weird behavior.

I'm sort of tempted to say that we should just clear the mark bits in beginMarkPhase. That's what we do for script data (in UnmarkScriptData). But it would have a cost, and maybe this way isn't so bad. I'll leave it up to you.

@@ +799,5 @@
> +        // stack is traced but is not in a zone being collected.
> +        //
> +        // Because of this we only treat the marked_ bit as a hint, and destroy
> +        // the RegExpShared if it was accidentally marked earlier but wasn't
> +        // marked by the current trace.

It would be useful to say why it's correct to do this. Something like "At worse, we end up with RegExpShareds that are marked even though they were never traced. The only danger here is that the source string they point to might be finalized, and we check for that."

::: js/src/vm/RegExpObject.h
@@ +408,5 @@
>          return createShared(cx, g);
>      }
>  
>      void setShared(ExclusiveContext *cx, RegExpShared &shared) {
> +        RegExpShared::writeBarrierPre(cx, &shared);

This isn't really right. A pre barrier is supposed to mark the *old* value in cases when we're updating. In this case I wonder if the old value is always null? Maybe try asserting that before making the change?

However, we also need to make sure that we trace any RegExpShareds that are created in the middle of an incremental GC. It's fine to use the same writeBarrierPre code for that, but I'd rather it be at the point of creation rather than the point of update.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #1)
> ::: js/src/jscompartment.cpp
> @@ +657,5 @@
> >  #endif
> >      JS_ASSERT(!debugScopes);
> >      JS_ASSERT(!gcWeakMapList);
> >      JS_ASSERT(enumerators->next() == enumerators);
> > +    JS_ASSERT(regExps.empty());
> 
> I'm just curious: what makes this true? It seems like off-thread parsing
> would create regexp objects, and I don't see anything that moves them over
> afterwards.

The RegExpShared structs are created on demand for a given RegExpObject (which stores the source/flags) and this is only done when the JS code actually executes (which has to happen on the main thread).

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

Updated patch.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32a1e7461250

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32a1e7461250