1109336 - Track more granular timings within root marking

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

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

See the lines beginning with *:

    Max Pause: 36.922
      Wait Background Thread: 0.003ms
      Purge: 0.010ms
      Mark: 27.849ms
        Other: 24.811ms
*       Unmark: 0.416ms
*       Mark Roots: 2.622ms
*         Mark Cross Compartment Wrappers: 0.001ms
*         Mark Rooters: 2.595ms
*         Mark Embedding: 0.001ms
      Sweep: 7.392ms
        Mark During Sweeping: 0.003ms
          Mark Incoming Black Pointers: 0.001ms
          Mark Weak: 0.001ms
          Mark Incoming Gray Pointers: 0.001ms
        Sweep Compartments: 6.753ms
          Sweep Discard Code: 0.031ms
          Sweep Inner Views: 0.001ms
          Sweep Cross Compartment Wrappers: 0.001ms
          Sweep Base Shapes: 0.009ms
          Sweep Initial Shapes: 0.045ms
          Sweep Type Objects: 0.037ms
          Sweep Regexps: 0.002ms
          Sweep Miscellaneous: 0.035ms
          Sweep type information: 6.669ms
            Other: 5.239ms
            Sweep type tables and compilations: 1.429ms
            Free type arena: 0.001ms
        Sweep Object: 0.011ms
        Sweep Script: 0.195ms
        Sweep Shape: 0.229ms
        Sweep JIT code: 0.181ms
        Deallocate: 0.002ms
      Minor GC: 156.590ms
        Other: 155.993ms
*       Mark Rooters: 0.597ms

Comment 1

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

Attachment: Track more granular root marking timings

I'm not sure whether it's worth the complexity. These are multiparented depending on whether they're in a minor or major GC.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8534002 [details] [diff] [review]
Track more granular root marking timings

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

::: js/src/gc/RootMarking.cpp
@@ +454,2 @@
>      if (traceOrMark == MarkRuntime) {
> +        gcstats::AutoPhase ap(stats, inMinorGC ? gcstats::PHASE_MARK_MINOR_CCWS : gcstats::PHASE_MARK_CCWS);

I feel like these should template out into a static and much shorter check:

template <bool isMinorGC> struct MultiHomedStat {
    static const int PHASE_MARK_CCWS = gcstats::PHASE_MARK_CCWS;
    ....
};
template <> struct MultiHomedState<true> {
    static const int PHASE_MARK_CCWS = gcstats::PHASE_MARK_MINOR_CCWS;
    ....
};

gcstats::AutoPhase ap(stats, MultiHomedStats<inMinorGC>::PHASE_MARK_CCWS);

Or somesuch.

Comment 3

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

(In reply to Terrence Cole [:terrence] from comment #2)
> Comment on attachment 8534002 [details] [diff] [review]
> Track more granular root marking timings
> 
> I feel like these should template out into a static and much shorter check:
> 
> template <bool isMinorGC> struct MultiHomedStat {
>     static const int PHASE_MARK_CCWS = gcstats::PHASE_MARK_CCWS;
>     ....
> };
> template <> struct MultiHomedState<true> {
>     static const int PHASE_MARK_CCWS = gcstats::PHASE_MARK_MINOR_CCWS;
>     ....
> };
> 
> gcstats::AutoPhase ap(stats, MultiHomedStats<inMinorGC>::PHASE_MARK_CCWS);

Er, I don't think that's going to work. inMinorGC is a bool, yes, but you are templatizing on its value and it isn't a constexpr. It isn't statically known. You'd have to do inMinorGC ? MHS<true>::PHASE_MARK_CCWS : MHS<false>::PHASE_MARK_CCWS or something, which defeats the purpose.

When doing the compaction timings, I had a gc::Statistics function for choosing between these sorts of multiparented phase ids, but it ended up being overkill. With these new additions, perhaps I should revisit that.

I guess if I assume that you'll always choose parents based on a single bool, it could be a little simpler.

Comment 4

[Terrence Cole [:terrence]]

Sorry, I left making isMinorGC static in MarkRuntime as an exercise for the reader. We should know whether we're calling MarkRuntime from the minor gc's callsite or not, so it should be trivial.

Comment 5

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

Attachment: Templatize markRuntime on inMinorGC

(In reply to Terrence Cole [:terrence] from comment #4)
> Sorry, I left making isMinorGC static in MarkRuntime as an exercise for the
> reader. We should know whether we're calling MarkRuntime from the minor gc's
> callsite or not, so it should be trivial.

Uh, ok, though markRuntime is pretty huge so that's a lot of code duplication for a handful of ternaries. But here is a delta on top of the previous patch that would do that; let me know what you think. (Note that defining the MajorMinorGC stuff in gc/Statistics.h is problematic because of include cycles.)

Comment 6

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

Attachment: Dynamic phase selection

Alternatively, here's the fully-dynamic option. This is actually more dynamic than it needs to be; the switch statement is me being lazy. I suppose I should be marking these f? instead of r?, but what path would you prefer? markRuntime duplication, fully dynamic, or a dynamic markRuntime calling majorMinorPhase without the switch statement?

Comment 7

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

Attachment: Templatize markRuntime on inMinorGC

One of the phases was wrong in the template version.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8534558 [details] [diff] [review]
Dynamic phase selection

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

Good point about the code duplication. I think I like this version more anyway.

Comment 10

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

Attachment: Track granular timings within Mark Roots

Phew. You wouldn't believe what I ran into in trying to get this to pass tests on all platforms.

Sorry for the complexity!

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8538695 [details] [diff] [review]
Track granular timings within Mark Roots

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

Wow, you weren't kidding about complexity. I really want these stats though; I'll probably regret it, but r=me.

Comment 12

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

Mostish recentish try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49336b89d508

https://hg.mozilla.org/integration/mozilla-inbound/rev/2efc03ac92f9

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2efc03ac92f9