Closed Bug 901178 Opened 7 years ago Closed 2 years ago

IonMonkey: IonBuilder::jsop_lambda should not create script of Lazy functions.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 918116

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(3 files)

I instrumented the trace logger to profile the time taken by the parsers, and noticed that during an IonCompilation, we are compiling the code of lazy functions on sunspider/date-format-tofte and sunspider/date-format-xparb.

The problem is that we call fun->getOrCreateScript(cx), which call the parser if we have a lazy script.  This approximately cost 0.5ms (~4%) on format-tofte and around 0.6ms (~4.5%) on format-xparb.

> 2850623808371223,start,ion_compile,tests/sunspider-1.0/date-format-tofte.js:8
> 2850623808410503,start,parser,tests/sunspider-1.0/date-format-tofte.js:45
> 2850623808410569,info,parser,lazy
> 2850623808436309,stop,parser
> 2850623808448515,start,parser,tests/sunspider-1.0/date-format-tofte.js:54
> 2850623808448531,info,parser,lazy
> 2850623808509801,stop,parser
> […]
> 2850623808956211,start,parser,tests/sunspider-1.0/date-format-tofte.js:253
> 2850623808956227,info,parser,lazy
> 2850623808972513,stop,parser
> 2850623808976961,start,parser,tests/sunspider-1.0/date-format-tofte.js:258
> 2850623808976977,info,parser,lazy
> 2850623809001153,stop,parser
> 2850623813341030,stop,ion_compile
- I think it might make sense to add the parser time to tracelogging?
- I think I don't totally understand. I thought we need to parse the script, if we want to compile the function? I'm not sure how you want to bypass that?
(In reply to Hannes Verschore [:h4writer] from comment #1)
> - I think it might make sense to add the parser time to tracelogging?

I will open another bug for that.
And probably another for adding the pid to the trace log file name, such as we can still use that for b2g or e10s.

> - I think I don't totally understand. I thought we need to parse the script,
> if we want to compile the function? I'm not sure how you want to bypass that?

We do, but we do not need to compile the lambdas. We only need to create a JSFunction which has a lazy script with a scope chain. There is no need to parse the script if we do not plan to use it.
Originally I had IonBuilder instantiate all the inner lambdas because LazyScript was a refcounted heap thing and cloning a function with a lazy script required bumping the refcount.  This is no longer the case, LazyScript is a GC thing, and cloning a function with a lazy script is the same amount of work and complexity as cloning a function with a real script.  So there's no longer a reason for the current behavior and this looks like a good fix.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Attached patch bug901178.patchSplinter Review
Ok, just removing the de-lazification show the following improvements on sunspider:

  date:                1.022x as fast     24.6ms +/- 0.4%    24.1ms +/- 0.4%     significant
    format-tofte:      1.017x as fast     12.5ms +/- 0.6%    12.3ms +/- 0.6%     significant
    format-xparb:      1.028x as fast     12.1ms +/- 0.6%    11.8ms +/- 0.5%     significant
Attachment #785995 - Flags: review?(bhackett1024)
Comment on attachment 785995 [details] [diff] [review]
bug901178.patch

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

::: js/src/ion/CodeGenerator.cpp
@@ +753,3 @@
>                    Address(output, JSFunction::offsetOfNativeOrScript()));
> +
> +    // Lamnda scripted function are given the scope chain as the environment in

"Lambda scripted functions"

::: js/src/ion/IonBuilder.cpp
@@ +8250,1 @@
>          return false;

This entire 'if' statement should be removed, as written it will cause IonBuilder to fail w/o an exception when the inner function is native (i.e. an asm.js module).

::: js/src/jsfun.h
@@ +328,5 @@
>      }
>  
> +    static unsigned offsetOfLazyScript() {
> +        return offsetof(JSFunction, u.i.s.lazy_);
> +    }

This function doesn't add anything, can you just JS_STATIC_ASSERT in offsetOfNativeOrScript() that u.i.s.lazy_ is at the same offset?
Attachment #785995 - Flags: review?(bhackett1024) → review+
Nice! I can see the xparb improvent. Somehow I don't see the tofte one, possible in the noise?
https://hg.mozilla.org/mozilla-central/rev/7d4e75a5d414
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(In reply to Hannes Verschore [:h4writer] from comment #7)
> Nice! I can see the xparb improvent. Somehow I don't see the tofte one,
> possible in the noise?

Comment 4 was ran with 1000 iterations.  So it is likely masked by the noise.
Depends on: 902253
Depends on: 902384
This bug causes issues with fuzzing and nightly builds, so backed out after discussing with nbp in-person:

https://hg.mozilla.org/integration/mozilla-inbound/rev/34921ae3f262
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla26 → ---
Currently the previous patch suffers for a problem which seems to be that during the compilation, the value of hasSingletonType might change, and it seems to cause trouble when calling types::UseNewTypeForClone in the Lowering.
Attachment #796418 - Flags: review?(bhackett1024)
(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> Currently the previous patch suffers for a problem which seems to be that
> during the compilation, the value of hasSingletonType might change, and it
> seems to cause trouble when calling types::UseNewTypeForClone in the
> Lowering.

Do you know what is causing the value of hasSingletonType to change?
(In reply to Brian Hackett (:bhackett) from comment #13)
> (In reply to Nicolas B. Pierron [:nbp] from comment #12)
> > Currently the previous patch suffers for a problem which seems to be that
> > during the compilation, the value of hasSingletonType might change, and it
> > seems to cause trouble when calling types::UseNewTypeForClone in the
> > Lowering.
> 
> Do you know what is causing the value of hasSingletonType to change?

Sorry, I badly identified the problem.  The problem is that we have a race between JSFunction::createScriptForLazilyInterpretedFunction and js::types::UseNewTypeForClone because we reset the flags before attaching the nonLazyScript version.

I will try to come-up with an additional patch to fix this issue.  We do not have this issue with IonBuilder because it is run sequentially.  I still think it makes sense to keep the decision made during IonBuilder as stable across the compilation.
As default of being simply able to prevent races (I would need a deeper understanding of all the cases), I am just adding this test which ensure that nobody else will fall in this trap after me.

I assumed that InParallelSection is a safe test to ensure that the runtime thread is blocked waiting for the completion of PJS synchronization.  Otherwise we want to ensure that the current thread can access the runtime and thus prevent any LazyScript -> JSScript convertion in the background.

This also means, that this patch cannot be landed without the second patch, as the current behavior of Ion *appears to* be racy, even if the script has already been de-lazified when the first patch is not applied.
Attachment #797095 - Flags: review?(bhackett1024)
Attachment #797095 - Flags: feedback?(shu)
Comment on attachment 797095 [details] [diff] [review]
Assert that we have no racy behavior on LazyScript convertion.

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

::: js/src/jsfun.cpp
@@ +1171,5 @@
> +JSFunction::noRaceOnScript() const
> +{
> +    return InParallelSection() || runtimeFromMainThread();
> +}
> +#endif

It's not immediately obvious that this logic is intended to be: if we are in PJS, then it's impossible to be de-lazifying a script; otherwise, we must be on the main thread. I would add a comment, at least for the PJS part.
Attachment #797095 - Flags: feedback?(shu) → feedback+
Attachment #796418 - Flags: review?(bhackett1024) → review+
Comment on attachment 797095 [details] [diff] [review]
Assert that we have no racy behavior on LazyScript convertion.

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

::: js/src/jsfun.cpp
@@ +1167,5 @@
>  }
>  
> +#ifdef DEBUG
> +bool
> +JSFunction::noRaceOnScript() const

I think a better name would be JSFunction::onValidThread.

@@ +1169,5 @@
> +#ifdef DEBUG
> +bool
> +JSFunction::noRaceOnScript() const
> +{
> +    return InParallelSection() || runtimeFromMainThread();

Threads with an ExclusiveContext can access the contents of functions in their thread-local zone, so this is wrong.  I think you can do:

return InParallelSection() || CurrentThreadCanAccessZone(compartment()->zone());
Attachment #797095 - Flags: review?(bhackett1024) → review+
Status: REOPENED → RESOLVED
Closed: 7 years ago2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 918116
You need to log in before you can comment on or make changes to this bug.