735449 - Add a describe op to JSClass for better heap dumping

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

We used to have some vestigial code in the heap dumper that tried to get extra info out of XPConnect. It seems like a good idea, and for some debugging I'm doing right now it's useful. This patch adds a describe hook to the JSClass so that XPConnect can add extra info about a given JSObject. Right now it just prints the address of the C++ object being wrapped. Can you think of anything else that would be useful here?

Comment 1

[Andrew McCreight [:mccr8]]

You could look at nsXPConnect::Traverse, in the section under cb.WantDebugInfo(). Did you mean feedback rather than review?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hmm, it seems like JS_DumpHeap already includes everything there except for GetScriptableInfo()->GetJSClass()->name. Do you know how that differs from clazz->name?

Comment 3

[Andrew McCreight [:mccr8]]

Sorry, I don't really know anything about that code. Eventually, I'd like to replace the Traverse code with something from the JS engine, hopefully shared with the JS heap dumping code.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

This also takes care of WrappedNativeProtos, and removes the custom xpconnect GC thing dumping.

Regarding the change to the reserved size. I talked to Waldo about this. Apparently this field ensures that JSClass is at least as large as js::Class. Then we define a pad field in js::Class to pad the size of js::Class up to JSClass. This ensures that JSClass is at least as big as js::Class. The whole thing is a little complicated, but I guess it makes sense.

Comment 5

[Andrew McCreight [:mccr8]]

This is scope creep, so don't worry about it here, but I wonder how much we can replace the existing callbacks-in-JSTracer with JSClass stuff.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 605600 [details] [diff] [review]
patch v2

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

Here are some comments for now.  It looks pretty good, I just have some nitpicky comments, and some thoughts on what the format should be. It would also be useful if you could give me some samples of what this new output looks like in the CC log and GC logs, if you have them handy.  r- because I'd like to see the patch after you address the comments/discuss the output format with me or whatever.

::: js/src/jsapi.cpp
@@ +2530,5 @@
>                      if (fun->atom) {
>                          *buf++ = ' ';
>                          bufsize--;
>                          PutEscapedString(buf, bufsize, fun->atom, 0);
>                      }

It kind of feels like this whole FunctionClass clause could be moved into a describe thing that is added to the FunctionClass clasp, but no big deal.  File a followup bug if you think it is a good idea, but too annoying to do in with this other stuff...

::: js/src/jsapi.h
@@ -3253,5 @@
>  
>  extern JS_PUBLIC_API(void)
>  JS_TraceRuntime(JSTracer *trc);
>  
> -#ifdef DEBUG

I think you should only move JS_GetTraceThing info out of the #ifdef DEBUG, as you don't add calls to the other functions. (likewise in the cpp file)

@@ +3430,5 @@
>      JSNative            construct;
>      JSHasInstanceOp     hasInstance;
>      JSTraceOp           trace;
>  
> +    void                *reserved[48];

Does 32 bit vs 64 bit matter here?  I don't really understand this part.  Maybe somebody else could look at it.

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +1066,5 @@
> +XPC_WN_Describe(JSRuntime *rt, JSObject *obj, char *buf, size_t size)
> +{
> +    nsISupports *p = static_cast<nsISupports *>(xpc_GetJSPrivate(obj));
> +    if (IS_SLIM_WRAPPER(obj)) {
> +        JS_snprintf(buf, size, "[WrappedNative(Slim) %p]", (void *)p);

I think "SlimWrappedNative" would be a little better.  It would also avoid parens which may confuse my hacky parsing regexps. Also maybe drop the braces and add a -?  Like "- SlimWrappedNative - %p" maybe?  It looks like that unlike the protos a wrapped native can have any name, so we need to leave this in.

@@ +1067,5 @@
> +{
> +    nsISupports *p = static_cast<nsISupports *>(xpc_GetJSPrivate(obj));
> +    if (IS_SLIM_WRAPPER(obj)) {
> +        JS_snprintf(buf, size, "[WrappedNative(Slim) %p]", (void *)p);
> +    } else {

"else if", not nested if. I think.

@@ +1069,5 @@
> +    if (IS_SLIM_WRAPPER(obj)) {
> +        JS_snprintf(buf, size, "[WrappedNative(Slim) %p]", (void *)p);
> +    } else {
> +        if (XPCWrappedNative *wrapper = static_cast<XPCWrappedNative *>(p))
> +            JS_snprintf(buf, size, "[WrappedNative %p]", (void *)wrapper->GetIdentityObject());

similar suggestion here and below about dropping the [] and adding a - in between.

@@ +1513,5 @@
>          mCanBeSlim = true;
>  
>      mJSClass.base.ext.isWrappedNative = true;
> +
> +    mJSClass.base.ext.describe = XPC_WN_Describe;

I think you should also set this in the definition of XPC_WN_NoHelper_JSClass. (just search for the other location of the string isWrappedNative in this file).

@@ +1697,5 @@
>  
> +static void
> +XPC_WN_Proto_Describe(JSRuntime *rt, JSObject *obj, char *buf, size_t size)
> +{
> +    XPCWrappedNativeProto *p = (XPCWrappedNativeProto *) xpc_GetJSPrivate(obj);

This should be a static_cast.  (Because this line is clearly too short already...)

@@ +1701,5 @@
> +    XPCWrappedNativeProto *p = (XPCWrappedNativeProto *) xpc_GetJSPrivate(obj);
> +    if (XPCNativeScriptableInfo *si = p->GetScriptableInfo())
> +        JS_snprintf(buf, size, "[WrappedNativeProto %s]", si->GetJSClass()->name);
> +    else
> +        JS_snprintf(buf, size, "[WrappedNativeProto]");

Do you need the WrappedNativeProto here?  I think the first part of JS_PrintTraceThingInfo (or whatever) will already contain the class name, such as XPC_WN_ModsAllowed_NoCall_Proto_JSClass.  It seems like the only new bit of information here is the scriptable info.  The braces don't seem to add much either.  Maybe just change the first case to " - %s" and drop the second one?

@@ +1704,5 @@
> +    else
> +        JS_snprintf(buf, size, "[WrappedNativeProto]");
> +}
> +
> +js::ClassExtension XPC_WN_Proto_Extentions = {

"Extensions" not "Extentions".

::: js/xpconnect/src/nsXPConnect.cpp
@@ +937,5 @@
>  
>      if (cb.WantDebugInfo()) {
>          char name[72];
> +        JS_GetTraceThingInfo(GetRuntime()->GetJSRuntime(), name, sizeof(name),
> +                             p, traceKind, true);

You should probably do the same "JS " prefix here, unless you want to roll that into JS_GetTraceThingInfo.  Well, never mind.  I guess it has [gc] before things that are JS things, so that should be good enough.  It will take a little getting used to, but no big deal.

Comment 8

[Andrew McCreight [:mccr8]]

So, I guess generally my thought on the format... currently, the CC log has entries like this:

0x1205929a0 [gc] JS Object (Function - schemaVersion)

So, I think they could be kind of like

0x1205929a0 [gc] Function - schemaVersion

I think this is mostly what your patch does.  Well, I guess the function case has to be changed slightly (add the dash).

The JS is implied by the [gc], so that's okay, and we don't really care that it is an object, assuming somebody doesn't have a confusing name for an object class.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v3

Here's a new patch.

The reserved thing doesn't depend on platform, since it's a void*. All that really matters is that it's "big enough". If it's not, the compiler will die with an error (which is what happened when I added the describe field). So there's not much that can go wrong here aside from a compiler error.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just tried to compile this and there are a few places where I didn't fix "extentions." Fixed locally. I must have really been on autopilot when I wrote that the first time.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v4

Found another bug so I guess I should re-post.

Comment 12

[Andrew McCreight [:mccr8]]

Could you paste a few examples of what this looks like into here?  Thanks.

Comment 13

[Andrew McCreight [:mccr8]]

For the GC and CC if you don't mind.  I'll try interpreting it from the raw printf but that makes it hard to get an overall sense of it.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 609579 [details] [diff] [review]
patch v4

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

::: js/src/jsfun.cpp
@@ +1110,5 @@
>      obj->toFunction()->trace(trc);
>  }
>  
>  static void
> +fun_describe(JSRuntime *rt, JSObject *obj, char *buf, size_t bufsize)

Did you drop the fun != obj case because it is just printing out a value that is going to be the field of the object?

Comment 15

[Andrew McCreight [:mccr8]]

Otherwise, the patch looks good to me.  I'd just like to see some example output as mentioned above before r+ing it.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'll get the output in a little while. My tree is messed up right now.

The fun != obj case is no longer possible. I think it got killed during objshrink. You can see by looking at the code for JSObject::toFunction. It's just a cast.

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 609579 [details] [diff] [review]
patch v4

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

r- just so I can see the example text.