877893 - PJS: Support basic string operations in parallel

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Should have at least concatenation and int and double conversion.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Part 1: Convert various string VM functions to take ThreadSafeContext

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Part 2: Support string concat in parallel in Ion

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Part 3: Convert VM functions needed for converting int/double to string to take ThreadSafeContext

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Part 4: Support MToString in parallel and add LDoubleToString

Comment 5

[Kannan Vijayan [:djvj]]

Comment on attachment 760729 [details] [diff] [review]
Part 2: Support string concat in parallel in Ion

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

::: js/src/ion/MIR.h
@@ +3540,5 @@
> +    }
> +
> +    static MParConcat *New(MDefinition *parSlice, MConcat *concat) {
> +        return New(parSlice, concat->lhs(), concat->rhs());
> +    }

This construction method seems redundant to me, why not just use |ins->lhs()| and |ins->rhs()| at the call site?

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 760731 [details] [diff] [review]
Part 4: Support MToString in parallel and add LDoubleToString

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

::: js/src/ion/CodeGenerator.cpp
@@ +591,5 @@
> +                        StoreRegisterTo(output));
> +        break;
> +      case ParallelExecution:
> +        ool = oolCallVM(ParallelIntToStringInfo, lir, (ArgList(), input),
> +                        StoreValueTo(AnyRegister(output)));

This should be able to be a StoreRegisterTo(output).

A callVM on a function with an output-MutableHandleString will call masm.popRooted(VMFunction::RootString, ReturnReg, JSReturnOperand), and the result will be stored in the ReturnReg.

StoreRegisterTo will perform a store from ReturnReg, whereas StoreValueTo performs a store from ValueOperand.

StoreValueTo's usage here seems incorrect.

@@ +632,5 @@
> +                        StoreRegisterTo(output));
> +        break;
> +      case ParallelExecution:
> +        ool = oolCallVM(ParallelDoubleToStringInfo, lir, (ArgList(), input),
> +                        StoreValueTo(AnyRegister(output)));

Same as above.  This should be StoreRegisterTo.

::: js/src/ion/IonBuilder.cpp
@@ +3253,5 @@
>          (left->type() == MIRType_String || right->type() == MIRType_String) &&
> +        (left->type() == MIRType_String ||
> +         left->type() == MIRType_Int32 || left->type() == MIRType_Double) &&
> +        (right->type() == MIRType_String ||
> +         right->type() == MIRType_Int32 || right->type() == MIRType_Double))

This conditional is super confusing and reflects intent poorly (not due to your changes in particular, but even the old version).

The following would be better:

if (op == JSOP_ADD &&
    ((left->type() == MIRType_String &&
        (right->type() == MIRType_String ||
         right->type() == MIRType_Int32 ||
         right->type() == MIRType_Double)) ||
     (left->type() == MIRType_Int32 &&
      right->type() == MIRType_String) ||
     (left->type() == MIRType_Double &&
      right->type() == MIRType_String)))
{
...
}

This would explain the captured cases a lot more clearly.

Comment 7

[Shu-yu Guo [:shu]]

(In reply to Kannan Vijayan [:djvj] from comment #6)
> Comment on attachment 760731 [details] [diff] [review]
> Part 4: Support MToString in parallel and add LDoubleToString
> 
> Review of attachment 760731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/CodeGenerator.cpp
> @@ +591,5 @@
> > +                        StoreRegisterTo(output));
> > +        break;
> > +      case ParallelExecution:
> > +        ool = oolCallVM(ParallelIntToStringInfo, lir, (ArgList(), input),
> > +                        StoreValueTo(AnyRegister(output)));
> 
> This should be able to be a StoreRegisterTo(output).
> 
> A callVM on a function with an output-MutableHandleString will call
> masm.popRooted(VMFunction::RootString, ReturnReg, JSReturnOperand), and the
> result will be stored in the ReturnReg.
> 
> StoreRegisterTo will perform a store from ReturnReg, whereas StoreValueTo
> performs a store from ValueOperand.
> 
> StoreValueTo's usage here seems incorrect.
> 

Good catch, I prepared this patch wrong I think, after I landed the non-Value handle outparam patch on central.

> @@ +632,5 @@
> > +                        StoreRegisterTo(output));
> > +        break;
> > +      case ParallelExecution:
> > +        ool = oolCallVM(ParallelDoubleToStringInfo, lir, (ArgList(), input),
> > +                        StoreValueTo(AnyRegister(output)));
> 
> Same as above.  This should be StoreRegisterTo.
> 
> ::: js/src/ion/IonBuilder.cpp
> @@ +3253,5 @@
> >          (left->type() == MIRType_String || right->type() == MIRType_String) &&
> > +        (left->type() == MIRType_String ||
> > +         left->type() == MIRType_Int32 || left->type() == MIRType_Double) &&
> > +        (right->type() == MIRType_String ||
> > +         right->type() == MIRType_Int32 || right->type() == MIRType_Double))
> 
> This conditional is super confusing and reflects intent poorly (not due to
> your changes in particular, but even the old version).
> 
> The following would be better:
> 
> if (op == JSOP_ADD &&
>     ((left->type() == MIRType_String &&
>         (right->type() == MIRType_String ||
>          right->type() == MIRType_Int32 ||
>          right->type() == MIRType_Double)) ||
>      (left->type() == MIRType_Int32 &&
>       right->type() == MIRType_String) ||
>      (left->type() == MIRType_Double &&
>       right->type() == MIRType_String)))
> {
> ...
> }
> 
> This would explain the captured cases a lot more clearly.

Agreed.

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Kannan Vijayan [:djvj] from comment #5)
> Comment on attachment 760729 [details] [diff] [review]
> Part 2: Support string concat in parallel in Ion
> 
> Review of attachment 760729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/MIR.h
> @@ +3540,5 @@
> > +    }
> > +
> > +    static MParConcat *New(MDefinition *parSlice, MConcat *concat) {
> > +        return New(parSlice, concat->lhs(), concat->rhs());
> > +    }
> 
> This construction method seems redundant to me, why not just use
> |ins->lhs()| and |ins->rhs()| at the call site?

It's been a convention that all the Par variants have a convenience constructor that takes the sequential variant.

Comment 9

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

Comment on attachment 760730 [details] [diff] [review]
Part 3: Convert VM functions needed for converting int/double to string to take ThreadSafeContext

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

::: js/src/jsnum.cpp
@@ +522,1 @@
>      if (JSFlatString *str = c->dtoaCache.lookup(10, si))

This seems a little scary to me. If the main thread were allowed to run at the same time, this would be problematic. Luke pointed out that that doesn't happen, so I guess it's safe. However, I doubt it helps very much, so I'd rather avoid it in the off-thread case, if only for readability. That would happen automatically if we don't have public access to ThreadSafeContext::compartment, which I'd like to see.

Comment 10

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

Comment on attachment 760728 [details] [diff] [review]
Part 1: Convert various string VM functions to take ThreadSafeContext

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

::: js/src/jsstr.h
@@ +22,5 @@
>  class JSStableString;
>  
>  namespace js {
>  
> +class ThreadSafeContext;

I think this shouldn't be needed with the jspubtd.h change.

::: js/src/vm/String-inl.h
@@ +173,5 @@
>               typename js::MaybeRooted<JSString*, allowGC>::HandleType right,
>               size_t length)
>  {
> +    JSContext *cxIfCanGC = allowGC ? tcx->toJSContext() : NULL;
> +    if (!validateLength(cxIfCanGC, length))

Can you change validateLength to take a ThreadSafeContext and call reportAllocationOverflow on it in the failure case? That seems cleaner.

::: js/src/vm/String.cpp
@@ +162,5 @@
>  }
>  
>  template<JSRope::UsingBarrier b>
>  JSFlatString *
> +JSRope::flattenInternal(ThreadSafeContext *maybetcx)

Can we assert at the top of this function that b == NoBarrier if maybetcx is not a JSContext?

::: js/src/vm/String.h
@@ +32,5 @@
>  namespace js {
>  
>  class StaticStrings;
>  class PropertyName;
> +class ThreadSafeContext;

..same

Comment 11

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/743204c6b245
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b9e08626e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/12e5bcffbbac
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/611624b7ddb4

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/743204c6b245
https://hg.mozilla.org/mozilla-central/rev/d6b9e08626e4
https://hg.mozilla.org/mozilla-central/rev/12e5bcffbbac
https://hg.mozilla.org/mozilla-central/rev/611624b7ddb4

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/332306bc7875