1154079 - [jsdbg2] Consider adding [[Class]] name to allocations log

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Should be easily accessible. Whether it is that useful is another question...

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add the allocated object's [[class]] name to the allocations log

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8591988 [details] [diff] [review]
Add the allocated object's [[class]] name to the allocations log

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

r=me with question about the metadata callback answered.

::: js/src/jscompartment.cpp
@@ +708,5 @@
>      objectMetadataTable = nullptr;
>  }
>  
>  void
> +JSCompartment::setNewObjectMetadata(JSContext* cx, HandleObject obj)

I'm unsure on the invariant requirements of the metadata callback though. Are both GC and JS reentry disallowed, or is only JS reentry disallowed? If GC is disallowed, why the change from JSObject* to HandleObject here?

::: js/src/vm/Debugger.h
@@ +258,5 @@
>      {
> +        AllocationSite(HandleObject frame, int64_t when, const char* className)
> +            : frame(frame)
> +            , when(when)
> +            , className(className)

I think SM still uses

: a(a),
  b(b)

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Shu-yu Guo [:shu] from comment #2)
> ::: js/src/jscompartment.cpp
> @@ +708,5 @@
> >      objectMetadataTable = nullptr;
> >  }
> >  
> >  void
> > +JSCompartment::setNewObjectMetadata(JSContext* cx, HandleObject obj)
> 
> I'm unsure on the invariant requirements of the metadata callback though.
> Are both GC and JS reentry disallowed, or is only JS reentry disallowed? If
> GC is disallowed, why the change from JSObject* to HandleObject here?

JS reentry is disallowed and GC is suppressed. I made the change because the handlers do things that could otherwise GC (if it weren't suppressed) and I think the hazard analysis will complain because someone could call the metadata handler outside of the AutoEnterAnalysis's AutoSuppressGC that it is usually called with. Seemed simpler than rooting at the top of SavedStacksMetadataCallback.

I guess an alternative would be to pass around a reference to the AutoEnterAnalysis (or its AutoSuppressGC?) that is up the stack to the metadata callback?

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> (In reply to Shu-yu Guo [:shu] from comment #2)
> > ::: js/src/jscompartment.cpp
> > @@ +708,5 @@
> > >      objectMetadataTable = nullptr;
> > >  }
> > >  
> > >  void
> > > +JSCompartment::setNewObjectMetadata(JSContext* cx, HandleObject obj)
> > 
> > I'm unsure on the invariant requirements of the metadata callback though.
> > Are both GC and JS reentry disallowed, or is only JS reentry disallowed? If
> > GC is disallowed, why the change from JSObject* to HandleObject here?
> 
> JS reentry is disallowed and GC is suppressed. I made the change because the
> handlers do things that could otherwise GC (if it weren't suppressed) and I
> think the hazard analysis will complain because someone could call the
> metadata handler outside of the AutoEnterAnalysis's AutoSuppressGC that it
> is usually called with. Seemed simpler than rooting at the top of
> SavedStacksMetadataCallback.
> 
> I guess an alternative would be to pass around a reference to the
> AutoEnterAnalysis (or its AutoSuppressGC?) that is up the stack to the
> metadata callback?

Since the object metadata hook always needs both GC and JS reentry suppressed, could you move the AutoEnterAnalysis to be inside the hook? If this is incorrect, just add an AutoSuppressGCAnalysis inside.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add the allocated object's [[class]] name to the allocations log

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=401feb6952b9

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add the allocated object's [[class]] name to the allocations log

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(That was just a tiny fix for the silly rooting hazard that showed up in the try push)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be627f24c4a

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1be627f24c4a