Refactor and simplify function relazification

RESOLVED FIXED in mozilla40

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

4 years ago
Some patches coming up to refactor function relazification, as we've been discussing for a while. In particular, I want to disentangle JSFunction::trace and JSFunction::relazify.

This will hopefully fix issues like bug 1005306.
(Assignee)

Comment 1

4 years ago
Created attachment 8600361 [details] [diff] [review]
Part 1 - Give JSFunction its own AllocKind

This turned out to be pretty straight-forward. It's a pretty large patch, but most if it is trivial renaming (JSFunction::FinalizeKind -> AllocKind::FUNCTION).
Attachment #8600361 - Flags: review?(terrence)
(Assignee)

Comment 2

4 years ago
Created attachment 8600366 [details] [diff] [review]
Part 2 - Add relazify phase

This patch adds a relazify-functions GC phase. It nicely simplifies JSFunction::trace and removes the scary incremental-GC comment + code in the relazification code.

I did some MOZ_GCTIMER measurements and loaded a bunch of JS-heavy pages (Gmail, Google Maps, TechCrunch, CNN etc.). The new phase is about as fast as " Mark Discard Code" most of the time; maybe a bit faster on average.

If this becomes a problem we can always restrict relazification to shrinking/compacting GCs.
Attachment #8600366 - Flags: review?(terrence)
(Assignee)

Comment 3

4 years ago
Created attachment 8600373 [details] [diff] [review]
Part 3 - Make LazyScript -> JSScript pointer weak

When we relazify, we currently do:

    if (lazy->maybeScript() == script)
        lazy->resetScript();

This can have complicated side-effects, although making relazification non-incremental (part 2) should have fixed most of the problems that I can think of.

This is a rebased version of the patch Till wrote for bug 1005306, with some additional (minor) fixes to make it green on Try. It makes the LazyScript -> JSScript pointer weak: we can collect a JSScript when all remaining references to it are from LazyScripts.

With this change, when we relazify a JSFunction, we only have to replace its JSScript with the LazyScript.

jonco reviewed the patch in bug 1005306 and answered questions on IRC today, requesting additional review from terrence because it's the kind of patch where an extra pair of eyes doesn't hurt.
Attachment #8600373 - Flags: review?(terrence)
Attachment #8600373 - Flags: review?(jcoppeard)
(Assignee)

Comment 4

4 years ago
FWIW, these patches are green on Try for the most part:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92b44877cb21

There's *some* orange though:

* An assert in XDRInterpretedFunction that's now bogus: it asserts that if we have a lazy function, its LazyScript has a nullptr JSScript*. This is no longer the case with part 3; I'll try to write a shell test for this.

* Some devtools tests timeout (browser_dbg_pretty-print*) on some platforms. Maybe these tests are creating a ton of functions; will look into this next week.
Comment on attachment 8600361 [details] [diff] [review]
Part 1 - Give JSFunction its own AllocKind

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

Nice!

::: js/src/frontend/Parser.cpp
@@ +1227,5 @@
>        default:
>          flags = JSFunction::INTERPRETED;
>          break;
>      }
>      

Please remove the whitespace while you're here.

::: js/src/jscompartment.cpp
@@ +756,5 @@
>      // currently stand in 1-1 relation with JSScripts; JSFunctions with the
>      // same LazyScript may create different JSScripts due to relazification of
>      // clones. See bug 1105306.
> +    for (gc::ZoneCellIter i(cx->zone(), AllocKind::FUNCTION); !i.done(); i.next()) {
> +        JSFunction* fun = &i.get<JSObject>()->as<JSFunction>())

\o/
Attachment #8600361 - Flags: review?(terrence) → review+
Comment on attachment 8600366 [details] [diff] [review]
Part 2 - Add relazify phase

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

Spectacular!
Attachment #8600366 - Flags: review?(terrence) → review+
Comment on attachment 8600373 [details] [diff] [review]
Part 3 - Make LazyScript -> JSScript pointer weak

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

This is a great improvement -- way less magical than what's there now.
Attachment #8600373 - Flags: review?(terrence) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 8600961 [details] [diff] [review]
Part 4 - Fix XDR

This patch fixes the XDR assert: we no longer null-out lazyScript->script_ when we relazify a function (see part 3), so the assert could fail (see the testcase).

I think the code works fine without the assert, so I removed it and added a comment to XDRLazyScript.
Attachment #8600961 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8600961 [details] [diff] [review]
Part 4 - Fix XDR

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

This sounds good :)
Attachment #8600961 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8600373 [details] [diff] [review]
Part 3 - Make LazyScript -> JSScript pointer weak

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

This looks fine to me.

::: js/src/jsscript.h
@@ +2010,2 @@
>      JSScript* maybeScript() {
> +        if (script_ && gc::IsAboutToBeFinalized(&script_))

We could use unbarrieredGet() here to avoid firing the barrier twice.
Attachment #8600373 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 11

4 years ago
Created attachment 8602083 [details] [diff] [review]
Part 5 - Fix CreateLazyScriptsForCompartment

As discussed before, this fixes the mochitest-dt timeouts.

Shu, before the patches in this bug, this happened:

(1) Set lazyScript->script to null when we relazify.
(2) CreateLazyScriptsForCompartment would see it's null and delazify the function.
(3) The function is non-lazy, the JSScript exists.

With the patches in this bug, *without* this fix:

(1) When we relazify we no longer set lazyScript->script to null.
(2) CreateLazyScriptsForCompartment will do nothing because it's non-null.
(3) A GC can collect the JSScript and set lazyScript->script to null (it's a weak pointer)
(4) The script is gone and the function is still lazy.

With this patch we delazify in CreateLazyScriptsForCompartment even if lazyScript->script is non-null.
Attachment #8602083 - Flags: review?(shu)
(Assignee)

Comment 13

4 years ago
(In reply to Pulsebot from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1c6a191fead0

Pushed part 1 because it's bitrot-prone.
Keywords: leave-open

Comment 15

4 years ago
Comment on attachment 8602083 [details] [diff] [review]
Part 5 - Fix CreateLazyScriptsForCompartment

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

Nice. Could you update the block comment above the ZoneCellIter loop, specifically the bit about "those which have not been compiled"? A word of warning about how a LazyScript having a JSScript doesn't necessarily mean it's delazified.
Attachment #8602083 - Flags: review?(shu) → review+
(Assignee)

Comment 17

4 years ago
Hm, if this sticks, we might be able to revert bug 996982 (I didn't realize it was fixed less than a month ago!). At least this problem:

  "JSFunctions with the same LazyScript may create different JSScripts due to relazification of clones."

Should be fixed by these patches as far as I know. Still, it's kinda nice to iterate over functions instead of scripts, because it has stronger invariants (no lazy functions left after we're done, instead of leaving clones lazy).
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1005306
(Assignee)

Comment 20

3 years ago
Forgot to close this after everything landed.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.