850026 - Optionally record information about allocation site when creating JS objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alexandre Poirot [:ochameau]]

In order to expose helpfull information in an eventual tool to track logical javascript leaks, it would be really great to be able to fetch allocation site of any given JSObject instance. So that when we are walking in GC graph and identify a suspicious JSObject we can easily point out to the developer what object is eventually leaking something.

nbp already built a patch to expose allocation site for a given js object,
but in the vast majority of cases, the site is empty or not defined.

In parallel, I'm trying to prototype what kind of tool we can come up with:
  http://blog.techno-barje.fr/post/2013/03/09/js-memory-debugging/
And such feature would greatly help exposing javascript object graph and understand it.

Comment 1

[Brian Hackett [Laid off!]]

Discussed briefly on IRC, but after bug 759585 lands there will be 32 bits of padding on 32 bit platforms in BaseShapes which could be used to store data about an object's allocation site or other metadata.  Doing this would increase shape fragmentation --- objects allocated in different places but with the same properties would have different shapes --- so we wouldn't want to always do it, but could make this an option on the cx or something and control it with a browser pref.  The increased fragmentation would hurt memory usage and caching effectiveness, but shouldn't change any object-object edges in the GC graph and affect the behavior of a leak finding tool.

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Alexandre Poirot (:ochameau) from comment #0)
> nbp already built a patch to expose allocation site for a given js object,
> but in the vast majority of cases, the site is empty or not defined.

Here is the brute-force attempt I made a few weeks ago:
https://github.com/nbp/mozilla-central/commits/t/alloc-location
https://github.com/nbp/mozilla-central/commit/861435e1990534b6677ff2ff654aaee8185420b2

Comment 3

[Jim Blandy :jimb]

(In reply to Brian Hackett (:bhackett) from comment #1)
> Discussed briefly on IRC, but after bug 759585 lands there will be 32 bits
> of padding on 32 bit platforms in BaseShapes which could be used to store
> data about an object's allocation site or other metadata.  Doing this would
> increase shape fragmentation --- objects allocated in different places but
> with the same properties would have different shapes --- so we wouldn't want
> to always do it, but could make this an option on the cx or something and
> control it with a browser pref.  The increased fragmentation would hurt
> memory usage and caching effectiveness, but shouldn't change any
> object-object edges in the GC graph and affect the behavior of a leak
> finding tool.

If we represent an allocation site as a JSScript and an offset, then perhaps we could replace the BaseShape compartment pointer with an AllocationSite pointer, which would then include the true compartment pointer. Then, for the non-collecting mode, all a compartment's BaseShapes could refer to the same fake AllocationSite built into the Compartment. There'd be no size hit, but there'd be an extra indirection.

Comment 4

[Alexandre Poirot [:ochameau]]

I'm really looking forward this allocation data for my memory tool.
I was wondering if it would be easy to craft a quick patch to do that, that doesn't necessary match review criteria but does the job. So that I can build a custom firefox and start playing with this data. And then if it proves to be usefull, we can eventually increase the importance of this bug.
I'm afraid writing it by myself is way beyond my capabilities... so any help would be much appreciated!

Comment 5

[Brian Hackett [Laid off!]]

Attachment: VM patch (d14e9efe0b00)

This patch adds a |metadata| attribute to objects that can be set or queried with the friend API, and adds an optional hook that can be called to provide metadata for every newly created object, based on the stack.  Under the hood this is implemented as another field in BaseShape analogous to |parent|.  This can fragment the shape tree if there are many different metadata objects around, but if the objects are tied to the allocation site then I think this will do alright.  Jim, do the jsfriendapi changes look like a workable interface?

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 751962 [details] [diff] [review]
VM patch (d14e9efe0b00)

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

Cool.  Looking forward to a heap profiler!

::: js/src/builtin/TestingFunctions.cpp
@@ +920,5 @@
> +    args.rval().setUndefined();
> +
> +    RootedObject obj(cx, &args[0].toObject());
> +    RootedObject metadata(cx, &args[1].toObject());
> +    return JSObject::setMetadata(cx, obj, metadata);

Could this use the jsfriendapi instead of the raw JSObject method?

@@ +932,5 @@
> +        JS_ReportError(cx, "Argument must be an object");
> +        return false;
> +    }
> +
> +    args.rval().setObjectOrNull(args[0].toObject().getMetadata());

ditto

Comment 7

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be1399f8f973

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Brian Hackett (:bhackett) from comment #7)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/be1399f8f973

Are the 3 functions be documented on MDN?

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Are the 3 functions be documented on MDN?

No, they are part of the friend API, whose contents aren't documented.  Actually exposing this to chrome code needs to be done in a followup.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be1399f8f973

Comment 11

[Jesse Ruderman]

What does this type of metadata let you do that WeakMap doesn't?

Comment 12

[Jim Blandy :jimb]

(In reply to Jesse Ruderman from comment #11)
> What does this type of metadata let you do that WeakMap doesn't?

It allows you to record data when objects are allocated, and it is efficient when there is a good correlation between shape and metadata.

Comment 13

[Jim Blandy :jimb]

Comment on attachment 751962 [details] [diff] [review]
VM patch (d14e9efe0b00)

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

This looks great for recording allocation locations.

There's no way to propagate OOM out of NewObjectMetadata; I would think it needs to be fallible. Ideally, I'd like to be able to propagate JS exceptions, too; while it would certainly be expensive to run JS on each allocation, I can imagine it being just the ticket for some analyses, and great for prototyping new ideas.

It's a shame we can't pass the new object to the metadata generation hook; I gather this is because we need the baseshape before we can create the object, and we use the metadata to choose the baseshape, so the object can't be in a complete state anyway, short of creating it with one baseshape, collecting metadata, and then substituting in a new baseshape. If we find an application that really wants the object, we can revisit the design.

Could we have tests that show that:

- metadata is properly retained as an object's shape changes?

- metadata is properly retained when objects enter dictionary mode? (Are there other interesting shape-related states like dictionary mode?)

- the recorded stack is correct? (The test could use 'evaluate' to specify the filename and line number, making it less fragile.)

- errors (OOM/slow script, at least; see the 'terminate' function in TestingFunctions.cpp) are correctly propagated?

Comment 14

[Varada]

In reply to Jim Blandy ( Comment 13 )
Please validate the below approach/points before I start with a trial implementation in my local build. Excuse my poor knowledge on the subject as just a few weeks back, started on the spider monkey internals and code
1) Solution to hookup the metadata to the object on creation so that it is run every time when objects are allocated at
NewObject(ExclusiveContext *cx, const Class *clasp, types::TypeObject *type_, JSObject *parent,
          gc::AllocKind kind, NewObjectKind newKind)
NewObjectWithType(JSContext *cx, HandleTypeObject type, JSObject *parent, gc::AllocKind allocKind,
                  NewObjectKind newKind = GenericObject)

is to
 
Make use of SetObjectMetadataCallback and provide a call back function

Where to use SetObjectMetadataCallback? I presume the callback function will collect filename and line number in the key:value pairs
 
2) Only enough to have a hook on jsobj.cpp and jsarray.cpp? No need for jsstr.cpp ( for strings), jsobj.cpp ( on bool and num object creation)? No need for jsdate.cpp, jsscope.cpp, jsfunc.cpp, globalobject.cpp as they call inturn jsobj.cpp for object creation? How about jsmath.cpp/jsscope.cpp for other objects/scope?

Comment 15

[Alexandre Poirot [:ochameau]]

Varada, I started using this feature in a prototype branch.
I'd suggest to look at it, as I'm using it from end to end and get a new working memory product:
  https://github.com/ochameau/mozilla-central/commit/812d9a239f5a0c04d06b193d712c738a91babf7c#diff-11

I've only tested this against b2g. It registers object metadata callback in app processes.
Then I've implement a new memory devtool actor, so that we can debug memory directly from the App manager.

To test this branch, you should build b2g out of this branch and flash your phone,
and also build Firefox with this same branch and open the app manager to connect to your phone.

I don't have much time to shape this patch to get it tested and reviewed,
but feel free to pick any part of it.