1373214 - MustSkipMarking results in unnecessary TLS lookups

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

MustSkipMarking is call a lot during marking and calls Zone::isGCMarking() which calls CurrentThreadIsHeapCollecting() which does a TLS lookup.  The current thread is always collecting if we are on this path though so we should refactor to remove this wasteful call.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1373214-is-gc-marking

My comment above is wrong: the current thread is not always collecting if marking is triggered by a barrier, but this seems to be the only place this is used outside of the collector.

When we're running the mutator the needsIncrementalBarrier flag is set if the zone is in either marking state or if we're running the pre barrier verifier.  When we're running the collector the needsIncrementalBarrier is false and the zone state is unchanged.

I factored out the calls that use this to know whether to mark something into shouldMarkInZone() and made this return true if either the GC state or the needsIncrementalFlag indicate marking is necessary.  

This works because inside the collector needsIncrementalBarrier is always false - so in this case the new version is equivalent to the old.  Outside the collector the zone is in a marking state only if needsIncrementalBarrier is set - so for this case too they are the same.

isGCMarking() now works in the same way as all the other isGCFoo() calls and is only called from the collector, hence no longer needs the check.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3112a3918ee5373a676173b58fbf430efa2891a&group_state=expanded&selectedJob=107342928

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8878085 [details] [diff] [review]
bug1373214-is-gc-marking

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

::: js/public/HeapAPI.h
@@ +167,5 @@
>      bool isGCMarkingGray() const { return gcState_ == MarkGray; }
>      bool isGCSweeping() const { return gcState_ == Sweep; }
>      bool isGCFinished() const { return gcState_ == Finished; }
>      bool isGCCompacting() const { return gcState_ == Compact; }
> +    bool isGCMarking() const { return gcState_ == Mark || gcState_ == MarkGray; }

Could this assert JS::CurrentThreadIsHeapCollecting()? Should it?

::: js/src/gc/Marking.cpp
@@ +344,2 @@
>  {
> +    MOZ_ASSERT(TenuredCell::fromPointer(thing)->zone()->shouldMarkInZone());

utterly irrelevant nit: I would expect thing->asTenured().zone()->... to avoid casting through a void*.

@@ +350,4 @@
>  {
>  #ifdef DEBUG
>      Zone* zone = TenuredCell::fromPointer(str)->zone();
> +    MOZ_ASSERT(zone->shouldMarkInZone() || zone->isAtomsZone());

Hm... fromPointer() seems to be the style in this file, I guess.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> > +    bool isGCMarking() const { return gcState_ == Mark || gcState_ == MarkGray; }
> 
> Could this assert JS::CurrentThreadIsHeapCollecting()? Should it?

That's a good question.  We're very careful about protecting the state in Zone proper, but shadow::Zone doesn't have any assertions for GC state.  I'll file a separate bug to investigate this.

> > +    MOZ_ASSERT(TenuredCell::fromPointer(thing)->zone()->shouldMarkInZone());
> 
> utterly irrelevant nit: I would expect thing->asTenured().zone()->... to
> avoid casting through a void*.

I would say that's a very relevant nit.  I'll fix for these functions.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64176330881
Change Zone::isGCMarking() to avoid a TLS lookup r=sfink

Comment 5

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

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

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

one of the changes that landed with this:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=370bb186e98f3fe5503efaa452a5689a052f8f86&tochange=d64176330881d9d9fb9b8f1192e57903c7ca5d2a

is responsible for some performance improvmenents!

== Change summary for alert #7386 (as of June 16 2017 10:55 UTC) ==

Improvements:

 13%  tp5o responsiveness linux64 opt e10s     6.18 -> 5.38
 13%  tp5o responsiveness linux64 opt e10s     6.18 -> 5.38
 13%  tp5o_webext responsiveness linux64 pgo e10s5.13 -> 4.46
 11%  tp5o responsiveness linux64 pgo e10s     4.95 -> 4.39
 11%  tp5o_webext responsiveness linux64 opt e10s6.33 -> 5.64
  5%  tps summary linux64 pgo e10s             26.24 -> 24.93

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7386

thanks for making firefox faster

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

is there any chance this would be turned off or not on mozilla-beta?  Possibly there is some condition where these improvements might be disabled/invalid based on other code/pref conditions in the browser?  I ask because we have some regressions on mozilla-beta which seem to be opposite of these improvements when screenshots was updated (bug 1392573).

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #7)
No, these changes are not conditional.