MustSkipMarking results in unnecessary TLS lookups

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks 1 bug)

55 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
Blocks: 1245279
(Assignee)

Comment 1

2 years ago
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
Assignee: nobody → jcoppeard
Attachment #8878085 - Flags: review?(sphink)
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.
Attachment #8878085 - Flags: review?(sphink) → review+
(Assignee)

Comment 3

2 years ago
(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

2 years ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64176330881
Change Zone::isGCMarking() to avoid a TLS lookup r=sfink

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d64176330881
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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

Updated

2 years ago
Depends on: 1389575
(Assignee)

Updated

2 years ago
No longer depends on: 1389575
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).
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 8

2 years ago
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #7)
No, these changes are not conditional.
Flags: needinfo?(jcoppeard)
You need to log in before you can comment on or make changes to this bug.