1460098 - Fix bitrot in gc/GCTrace.cpp

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Paul Bone [:pbone]]

I'm working on a change to improve JS_GC_TRACE, I'll split it into two separate features, tracing for allocation/moves/finalisation and tracing for the GC state.  I want to do some tracing and I only care about GC state so I don't need to see the other messages.  I may also add more state messages.

Comment 1

[Paul Bone [:pbone]]

Attachment: Attempt to fix bitrot in GCTrace.cpp

So I started to fix some of the bitrot in GCTrace.cpp.  I found that this might not be what I wanted after all, since it's a binary tracing format. do we have the tools to read the output and process it / display it nicely?  I was actually after some simple logging code and remember seeing it previously, so this is less important than I had imagined.  Nevertheless tidying it up isn't bad.

I ran into two problems:

1. I broke the JIT:

   Assertion failure: masm.framePushed() == frameSize(), at /home/paul/dev/moz/avoid-minor-gc/js/src/jit/CodeGenerator.cpp:3030

   I could look at this further but it's also the end of my work day.  and since I might not need GCTrace anyway it might not be worth-while.

2. Some of the GCTrace actions require a RAII object to show that sweeping has been performed.  I'm not sure that this can be done easily, again, it might not be worth-while.

Jon: I remember mutterings about throwing away this code? what do you think?

Ted: I can take a quick look at the JIT on Monday, but you probably know within a few seconds what I'm doing/not doing here.

Cheers.

Comment 2

[Ted Campbell [:tcampbell]]

Comment on attachment 8974903 [details] [diff] [review]
Attempt to fix bitrot in GCTrace.cpp

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

::: js/src/gc/GCTrace.cpp
@@ +174,5 @@
>      MaybeTraceClass(group->clasp());
>      TraceEvent(TraceEventGroupInfo, uint64_t(group));
>      TraceAddress(group->clasp());
> +    // TODO: Can't call flags without sweeping.
> +    // TraceInt(group->flags());

|group->flagsDontCheckGeneration()| will work for your needs.

@@ +189,5 @@
>      static char buffer[bufLength];
> +     */
> +
> +    /*
> +     * It looks like we can't do anything with newScript without sweeping.

|newScriptDontCheckGeneration|

Not sweeping is only a concern if you hold on to results such as ObjectGroups. Getting just the JSAtom will be fine.

@@ +208,5 @@
>  
>  void
>  js::gc::TraceCreateObject(JSObject* object)
>  {
> +    AutoUnsafeCallWithABI unsafe;

Thanks!

::: js/src/jit/MacroAssembler.cpp
@@ +1456,4 @@
>      }
>  
>  #ifdef JS_GC_TRACE
> +    LiveRegisterSet regs = LiveRegisterSet(RegisterSet::Volatile());

Let's use same style as https://searchfox.org/mozilla-central/rev/babf96cf0c37b608e9663e2af73a4d21819c4af1/js/src/jit/MacroAssembler.cpp#2066-2077

@@ +1460,1 @@
>      regs.takeUnchecked(obj);

Reorder this back to the original should solve your assertion failure.

PushRegsInMask;
regs.takeUnchecked
regs.takeAnyGeneral

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #1)
> do
> we have the tools to read the output and process it / display it nicely? 

Not exactly.  The code under js/src/devtools/gctrace reads a log and outputs several data files based on it.

> Jon: I remember mutterings about throwing away this code? what do you think?

I thnought this was totally unused but I think Steve had a ues for it when I proposed deleting it recently.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8974903 [details] [diff] [review]
Attempt to fix bitrot in GCTrace.cpp

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

The changes look fine.  Whether this will solve your tracing needs is another question :)

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1460098 (Part 1) - Fix bitrot in GCTrace.cpp

Hi Steve,

I'm asking you to review this series of patches because Jon said you might have a use for this code.

Currently it's a series of patches because that may be easier to review, you can see one step at a time what I've done.  But at least some of it should probably be folded together for landing, although each should work (better than before) on its own.

Comment 6

[Paul Bone [:pbone]]

Attachment: Bug 1460098 (Part 2) - Rename AutoInUnsafeCallWithABI -> AutoUnsafeCallWithABI in a string

Comment 7

[Paul Bone [:pbone]]

Attachment: Bug 1460098 (Part 3) - Move GC tracing into a class

Comment 8

[Paul Bone [:pbone]]

Attachment: Bug 1460098 (Part 4) - Use newScriptDontCheckGeneration

Comment 9

[Paul Bone [:pbone]]

Attachment: Bug 1460098 (Part 5) - Move GCTrace's state into the class

Comment 10

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

Comment on attachment 8976151 [details] [diff] [review]
Bug 1460098 (Part 3) - Move GC tracing into a class

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

::: js/src/gc/GCTrace.h
@@ +16,5 @@
>  namespace gc {
>  
> +/*
> + * Tracing code is declared within this class, so the class can be a friend of
> + * something and access private details used for tracing

nit: end comment with a period.

Comment 11

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

Comment on attachment 8976152 [details] [diff] [review]
Bug 1460098 (Part 4) - Use newScriptDontCheckGeneration

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

::: js/src/gc/GCTrace.cpp
@@ +171,5 @@
>      MOZ_ALWAYS_TRUE(tracedClasses.put(clasp));
>  }
>  
> +void
> +js::gc::GCTrace::maybeTraceGroup(ObjectGroup* group)

This makes me wonder why the entire body of this file isn't in namespace js { namespace gc {.

Comment 12

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

Comment on attachment 8976153 [details] [diff] [review]
Bug 1460098 (Part 5) - Move GCTrace's state into the class

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

::: js/src/gc/GCTrace.cpp
@@ +34,2 @@
>  static inline void
> +WriteWord(FILE *file, uint64_t data)

It seems like this could go further and move WriteWord, TraceEvent, etc. into gcTracer and grab the FILE* from there, but this is still an improvement.

Comment 13

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

Comment on attachment 8976148 [details] [diff] [review]
Bug 1460098 (Part 1) - Fix bitrot in GCTrace.cpp

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

::: js/src/gc/GCTrace.cpp
@@ +133,4 @@
>          /* We don't have AllocKind here, but we can work it out from size. */
>          unsigned slots = (size - sizeof(JSObject)) / sizeof(JS::Value);
>          AllocKind kind = GetBackgroundAllocKind(GetGCObjectKind(slots));
> +        TraceEvent(TraceEventNurseryAlloc, uint64_t(thing), uint8_t(kind));

I guess this is the way things are set up now, but all this casting seems kind of awkward. But I guess it would be a bit of a nuisance to do

void TraceEventNurseryAlloc(Cell* cell, AllocKind kind)
{
    TraceEvent(TraceEvent::NurseryAlloc, uint64_t(cell), uint8_t(kind));
}

for all the different types of events.

@@ +175,5 @@
>      TraceEvent(TraceEventGroupInfo, uint64_t(group));
>      TraceAddress(group->clasp());
> +    // TODO: Can't call flags without sweeping.
> +    // TraceInt(group->flagsDontCheckGeneration());
> +    abort(); // temporary until we fix group->flags();

I assume this gets fixed in a later patch?

This doesn't make things better with only the partial stack applied, then, I guess. Oh well; I'm fine with reviewing it like this if you squash before landing. We don't want bisectors to hit changesets that abort().

::: js/src/gc/GCTraceFormat.h
@@ +24,4 @@
>      TraceEventTenuredAlloc,
>      TraceEventClassInfo,
>      TraceEventTypeInfo,
> +    TraceEventGroupInfo,

What is TraceEventTypeInfo now? Does it still need to exist?

Comment 14

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

I do really need to look at this tracing stuff to understand it and see whether it's useful to me or not.

Comment 15

[Paul Bone [:pbone]]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #11)
> Comment on attachment 8976152 [details] [diff] [review]
> Bug 1460098 (Part 4) - Use newScriptDontCheckGeneration
> 
> Review of attachment 8976152 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/GCTrace.cpp
> @@ +171,5 @@
> >      MOZ_ALWAYS_TRUE(tracedClasses.put(clasp));
> >  }
> >  
> > +void
> > +js::gc::GCTrace::maybeTraceGroup(ObjectGroup* group)
> 
> This makes me wonder why the entire body of this file isn't in namespace js
> { namespace gc {.

Doh, I meant to come back and do that but guess I forgot.  I'll add that.

Comment 16

[Paul Bone [:pbone]]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #12)
> Comment on attachment 8976153 [details] [diff] [review]
> Bug 1460098 (Part 5) - Move GCTrace's state into the class
> 
> Review of attachment 8976153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/GCTrace.cpp
> @@ +34,2 @@
> >  static inline void
> > +WriteWord(FILE *file, uint64_t data)
> 
> It seems like this could go further and move WriteWord, TraceEvent, etc.
> into gcTracer and grab the FILE* from there, but this is still an
> improvement.

I made that choice deliberately.  Call me strange but I'm not really onboard with C++'s exposing inner details like private methods in header files so that if you change them the whole project needs to rebuild.  I understand why it is that way, because C++ doesn't have an actual module system etc.  So I kept these as static methods where even their existence is a hidden implementation detail.

Comment 17

[Paul Bone [:pbone]]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #13)
> Comment on attachment 8976148 [details] [diff] [review]
> Bug 1460098 (Part 1) - Fix bitrot in GCTrace.cpp
> 
> Review of attachment 8976148 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +175,5 @@
> >      TraceEvent(TraceEventGroupInfo, uint64_t(group));
> >      TraceAddress(group->clasp());
> > +    // TODO: Can't call flags without sweeping.
> > +    // TraceInt(group->flagsDontCheckGeneration());
> > +    abort(); // temporary until we fix group->flags();
> 
> I assume this gets fixed in a later patch?

Yes it does.

> This doesn't make things better with only the partial stack applied, then, I
> guess. Oh well; I'm fine with reviewing it like this if you squash before
> landing. We don't want bisectors to hit changesets that abort().

No, it's still better, now this code compiles (when JS_GC_TRACE) is on, it just aborts, without that flag it won't abort().  Anyway, I will still squash these together.

> ::: js/src/gc/GCTraceFormat.h
> @@ +24,4 @@
> >      TraceEventTenuredAlloc,
> >      TraceEventClassInfo,
> >      TraceEventTypeInfo,
> > +    TraceEventGroupInfo,
> 
> What is TraceEventTypeInfo now? Does it still need to exist?

Hrm, you're probably right, it's probably unused.  I'll check.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37dc32d486bd
(Part 1) - Fix bitrot in GCTrace.cpp r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad64ea702de0
(Part 2) - Rename AutoInUnsafeCallWithABI -> AutoUnsafeCallWithABI r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/9721f9a45fda
(Part 3) - Move GCTrace's state into the class r=sfink

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a37d20848944
Fix undefined symbol in NoOpt builds on a CLOSED TREE r=bustage

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/37dc32d486bd
https://hg.mozilla.org/mozilla-central/rev/ad64ea702de0
https://hg.mozilla.org/mozilla-central/rev/9721f9a45fda
https://hg.mozilla.org/mozilla-central/rev/a37d20848944