906060 - Allow ExclusiveContext zones to have TI enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch (b5e301863e69)

Right now we only parse scripts referred to by XUL documents off thread, which will never have TI enabled.  Operations with an ExclusiveContext do not currently support TI, and this restriction needs to be removed in order to allow scripts referred to by HTML documents to be parsed off thread.  The attached patch does this, and is largely straightforward s/JSContext/ExclusiveContext/.

Comment 1

[Nicolas B. Pierron [:nbp]]

Can you fix Bug 900681, such as I do not have to backout this patch too?

Comment 2

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

Comment on attachment 791301 [details] [diff] [review]
patch (b5e301863e69)

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

I don't know much about TI. Could you explain the stuff below, or maybe get Jan to review these parts? Do we not have to worry about constraints because Ion isn't running off the main thread? If so, it would be nice if we could factor the constraint stuff into a separate function and put a comment there explaining the thread stuff.

::: js/src/jsgc.cpp
@@ +4057,5 @@
>      runtime->gcNumber++;
> +
> +#ifdef DEBUG
> +    for (ThreadDataIter iter(rt); !iter.done(); iter.next())
> +        JS_ASSERT(!iter->suppressGC);

What guarantees this? The fact that threads won't pause for GC during TI? If so, please write a comment.

::: js/src/jsinfer.cpp
@@ +1856,5 @@
> +            TypeConstraint *constraint = types->constraintList;
> +            while (constraint) {
> +                constraint->newObjectState(cx, object, force);
> +                constraint = constraint->next;
> +            }

?

@@ +3644,5 @@
> +            TypeConstraint *constraint = types->constraintList;
> +            while (constraint) {
> +                constraint->newObjectState(cx, this, true);
> +                constraint = constraint->next;
> +            }

?

@@ +3751,5 @@
>       * script keeps track of where each property is initialized so we can walk
>       * the stack and fix up any such objects.
>       */
>      Vector<uint32_t, 32> pcOffsets(cx);
> +    Maybe<ScriptFrameIter> iter;

Can we avoid the Maybe<> business by wrapping the whole loop in |if (cx->isJSContext())|? That seems clearer to me (although admittedly this version has a simpler diff :-).

@@ +3753,5 @@
>       */
>      Vector<uint32_t, 32> pcOffsets(cx);
> +    Maybe<ScriptFrameIter> iter;
> +    if (cx->isJSContext())
> +        iter.construct(cx->asJSContext());

Why don't we have to run this loop off the main thread?

::: js/src/jsinferinlines.h
@@ +1345,5 @@
> +        while (constraint) {
> +            cx->compartment()->types.addPending(cx, constraint, this, type);
> +            constraint = constraint->next;
> +        }
> +        cx->compartment()->types.resolvePending(cx);

?

@@ +1367,5 @@
> +        TypeConstraint *constraint = constraintList;
> +        while (constraint) {
> +            constraint->newPropertyState(cx, this);
> +            constraint = constraint->next;
> +        }

?

::: js/src/vm/Runtime.cpp
@@ -400,5 @@
>      JS_ASSERT(!exclusiveAccessOwner);
>      if (exclusiveAccessLock)
>          PR_DestroyLock(exclusiveAccessLock);
>  
> -    JS_ASSERT(!numExclusiveThreads);

Why can't we assert this anymore?

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #2)
> Comment on attachment 791301 [details] [diff] [review]
> patch (b5e301863e69)
> 
> Review of attachment 791301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't know much about TI. Could you explain the stuff below, or maybe get
> Jan to review these parts? Do we not have to worry about constraints because
> Ion isn't running off the main thread? If so, it would be nice if we could
> factor the constraint stuff into a separate function and put a comment there
> explaining the thread stuff.

Constraints aren't generated until we analyzeTypes (which just happens when invoking scripts with 'new' until 906788 is fixed, after which it won't happen at all) and when Ion compiling code.  Since the compartment/zone used by the exclusive context haven't had any code execute in them, neither of these will hold.

> ::: js/src/jsgc.cpp
> @@ +4057,5 @@
> >      runtime->gcNumber++;
> > +
> > +#ifdef DEBUG
> > +    for (ThreadDataIter iter(rt); !iter.done(); iter.next())
> > +        JS_ASSERT(!iter->suppressGC);
> 
> What guarantees this? The fact that threads won't pause for GC during TI? If
> so, please write a comment.

Yes, I'll write a comment.

> @@ +3753,5 @@
> >       */
> >      Vector<uint32_t, 32> pcOffsets(cx);
> > +    Maybe<ScriptFrameIter> iter;
> > +    if (cx->isJSContext())
> > +        iter.construct(cx->asJSContext());
> 
> Why don't we have to run this loop off the main thread?

This loop is inspecting code that is on the stack, and since no code has executed in the exclusive context's compartment there won't be any frames found.

> ::: js/src/vm/Runtime.cpp
> @@ -400,5 @@
> >      JS_ASSERT(!exclusiveAccessOwner);
> >      if (exclusiveAccessLock)
> >          PR_DestroyLock(exclusiveAccessLock);
> >  
> > -    JS_ASSERT(!numExclusiveThreads);
> 
> Why can't we assert this anymore?

I think this was working around the problem fixed by bug 900681, I'll remove this change.

Comment 4

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

Comment on attachment 791301 [details] [diff] [review]
patch (b5e301863e69)

> > Why don't we have to run this loop off the main thread?
> 
> This loop is inspecting code that is on the stack, and since no code has executed in the
> exclusive context's compartment there won't be any frames found.

Can we put an assertion here? It looks like PerThreadData has an activations_ field. Maybe we could assert that it's NULL?

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a155905a9d08

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a155905a9d08