906091 - GC: MarkGlobalForMinorGC() slows down root marking for minor GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

In a browser build, by far the greatest contribution to the runtime of MarkRuntime() in a minor GC is calling MarkGlobalForMinorGC().  This marks all the non-reserved slots of the global for every compartment in the system.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: minor-marking.log

This breakdown shows times in microseconds for the various parts of MarkRuntime().  The first column "Minor" is the total, and "GlobMin" is the result of calling MarkGlobalForMinorGC().

Comment 2

[Jon Coppeard (:jonco)]

The comments in MarkGlobalForMinorGC() indicate that this is done to avoid barriers on writes to the global's slots.  Would it be possible to add these barriers, or would that have its own performance penalty?

Comment 3

[Brian Hackett [Laid off!]]

I avoided the barriers on global object slots because one of the octane benchmarks (earley boyer) I think was just adding a ton of entries for global writes and exhausting the store buffer very quickly.  I thought in the browser context this was just done for compartments with Ion code, but maybe that's changed.  In any case there are other options for dealing with this --- try to avoid adding store buffer entries for globals when they are already known to be in the store buffer, or keep more precise track of the globals or even {global,slot} tuples which Ion writes to without post barriers.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: workInProgress

Patch to add globals to the storebuffer on the first write only.

I can tidy this up and un-duplicate the LIR/MIR nodes by setting a flag if the object is a global.

I saw some crashes with this in the browser, but I haven't worked out whether they are existing issues.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: workInProgress

qref'd patch this time

Comment 6

[Jon Coppeard (:jonco)]

Attachment: post-barrier-global

Post barrier writes to globals, but add a flag in JSCompartment to only put cell in the store buffer once. 

MarkRuntime times are much reduced and v8 benchmarks are unaffected.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: minor-post.log

Some MarkRuntime timings with patch applied.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 808649 [details] [diff] [review]
post-barrier-global

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

Very nice! r=me

::: js/src/jscompartment.h
@@ +205,5 @@
>  
>      /* Set of all currently living type representations. */
>      js::TypeRepresentationHash   typeReprs;
>  
> +    bool                         globalWriteBarriered;

This could use a fairly detailed comment explaining what state it is tracking and why.

Comment 9

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6942eeef1a

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/6d6942eeef1a