Comment on attachment 8484655 [details] [diff] [review] Implement JS::ubi::Node::size for JSObjects. Review of attachment 8484655 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ +1999,5 @@ > + JS::AutoCheckCannotGC autoCannotGC; > + > + JS::ubi::Node node = args.get(0); > + if (node) > + args.rval().set(NumberValue(node.size(mallocSizeOf))); while you're here, can this be args.rval().setNumber(...)? ::: js/src/jit-test/tests/heap-analysis/byteSize-of-object.js @@ +13,5 @@ > + } > + print(); > +} > + > +expandObj({}); I wonder if there's any value in wrapping this sequence of expandObj calls in a loop: for (_ of [1,2]) { ...expandObj... gc; } to check both nursery and tenured cases. ::: js/src/jsobj.cpp @@ +6441,5 @@ > + > + // We can only use addSizeOfExcludingThis on tenured objects, as it assumes > + // it can apply mallocSizeOf to bits and pieces of the object, whereas > + // objects in the nursery may have some of their bits and pieces > + // allocated... in the nursery, which is a giant block of non-malloc memory. Seems a little too ellipsistic. @@ +6443,5 @@ > + // it can apply mallocSizeOf to bits and pieces of the object, whereas > + // objects in the nursery may have some of their bits and pieces > + // allocated... in the nursery, which is a giant block of non-malloc memory. > + // What we return here is wrong; see bug 1062470. > + if (IsInsideNursery(&obj)) { It almost seems like you could make this if (IsInsideNursery(&obj) && !(obj.hasDynamicSlots() || obj.hasDynamicElements())) but that's kind of intertwingled, isn't it? Never mind.

This is used by the revised tests for JSObject sizing; see the comments there before reviewing.

I'm asking for re-review of this patch, since I've added tests and changed some of the code. The testing strategy here is the one suggested by jorendorff. It may be controversial; the comments in the test make the case.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39c3f06885b2

Comment on attachment 8561257 [details] [diff] [review] Implement JS::ubi::Node::size for JSObjects. Review of attachment 8561257 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsobj.cpp @@ +4096,5 @@ > + if (!elements.isCopyOnWrite() || elements.ownerObject() == this) > + size += elements.capacity * sizeof(HeapSlot); > + } > + } else { > + size = sizeof(JSObject); Drive-by comment: typed objects and unboxed objects are non-native objects bigger than this. Especially the latter will become quite popular in normal JS code when we enable them by default. See UnboxedPlainObject in vm/UnboxedObject.h.

Comment on attachment 8561257 [details] [diff] [review] Implement JS::ubi::Node::size for JSObjects. Review of attachment 8561257 [details] [diff] [review]: ----------------------------------------------------------------- (Sorry to hop on the drive-by bandwagon) ::: js/src/jsobj.cpp @@ +4096,5 @@ > + if (!elements.isCopyOnWrite() || elements.ownerObject() == this) > + size += elements.capacity * sizeof(HeapSlot); > + } > + } else { > + size = sizeof(JSObject); Are TypedArrays considered a special case of "typed objects"? Are they handled by this code?

Comment on attachment 8561255 [details] [diff] [review] Add pointerByteSize function to JS shell. Review of attachment 8561255 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/heap-analysis/pointerByteSize.js @@ +1,2 @@ > +// Try out the pointerByteSize shell function. > +assertEq(pointerByteSize() == 4 || pointerByteSize() == 8, true); This is why I'd like to have an inline 'of' operator. assertEq(pointerByteSize() of [4, 8], true); except that it reads badly. Really, I want 'in' to be something sane, but that's a long lost cause. ::: js/src/shell/js.cpp @@ +4624,5 @@ > " Return true iff the string's characters are stored as Latin1."), > > + JS_FN_HELP("pointerByteSize", PointerByteSize, 0, 0, > +"pointerByteSize()", > +" Return the size of a pointer in the current process, in bytes."), Would it be too much to ask for this to be moved to getBuildConfiguration() instead? I'm getting overwhelmed by the number of shell functions these days. http://dxr.mozilla.org/mozilla-central/source/js/src/builtin/TestingFunctions.cpp#58

Comment on attachment 8561257 [details] [diff] [review] Implement JS::ubi::Node::size for JSObjects. Review of attachment 8561257 [details] [diff] [review]: ----------------------------------------------------------------- Sorry terrence, but redirecting to you for the nursery size stuff. ::: js/src/jit-test/tests/heap-analysis/byteSize-of-object.js @@ +3,5 @@ > +// We actually hard-code specific sizes into this test, even though they're > +// implementation details, because in practice there are only two architecture > +// variants to consider (32-bit and 64-bit), and if these sizes change, that's > +// something SpiderMonkey hackers really want to know; they're supposed to be > +// stable. Are they? This test is terrifying. Will this survive unboxed objects? @@ +22,5 @@ > + var postGCSize = byteSize(obj); > + if (preGCSize == postGCSize) > + return preGCSize; > + return 0; > +} Sorry, what is this doing? Does the t in the name mean tenuredByteSize? (And if so, why return preGCSize instead of postGCSize? Not that it matters given that they're the same, but if this *is* supposed to stand for tenuredByteSize, it seems weird.) It seems like you're making this fail if tenuring changes an object's size. Is that right, and if so, why? @@ +26,5 @@ > +} > + > +assertEq(tByteSize({}), s(48, 64)); > + > +assertEq(tByteSize({ w: 1 }), s(32, 48)); In general, considering the craziness of what this test is, I would much prefer jimb-style comments. This section should say it's testing objects with only named properties. Why is an empty object larger than one with a single field? Would it be worth adding a bunch of things like const FINALIZE_OBJECT2 = s(n32, 64); at the beginning and using those in the asserts? (Or a made-up SIZE_OBJECT2) That would help explain why 3 properties use the same size as 4 properties. @@ +40,5 @@ > +assertEq(tByteSize({ 0:0, 1:1, 2:2, 3:3, 4:4 }), s(148, 168)); > + > +// Mix indexed and named properties, exploring each combination of the size > +// classes above. Oddly, the changes here as the objects grow are not simply the > +// sums of the changes above. Perhaps this craziness prevents the SIZE_OBJECT2 stuff. @@ +52,5 @@ > +assertEq(tByteSize({ w:1, x:2, y:3, z:4, a:6, 0:0, 1:1, 2:2 }), s(148, 168)); > +assertEq(tByteSize({ w:1, x:2, y:3, z:4, a:6, 0:0, 1:1, 2:2, 3:3, 4:4 }), s(180,200)); > + > + > +assertEq(byteSize([]), s(16, 32)); I wonder if this can fail in any of the zeal builds if you happen to get a GC at just the wrong time. ::: js/src/jsobj.cpp @@ +4094,5 @@ > + if (native.hasDynamicElements()) { > + js::ObjectElements &elements = *native.getElementsHeader(); > + if (!elements.isCopyOnWrite() || elements.ownerObject() == this) > + size += elements.capacity * sizeof(HeapSlot); > + } This seems good for PlainObjects. I'm not convinced this is adequate for other NativeObjects. ArrayBufferObjects will never be in the nursery, so they're ok. TypedArrayObjects with inlined data (and therefore lazy buffers) have some data after the slots & elements. I honestly don't know how they're handled by the nursery. I think I'll have to 302 terrence for that. @@ +4096,5 @@ > + if (!elements.isCopyOnWrite() || elements.ownerObject() == this) > + size += elements.capacity * sizeof(HeapSlot); > + } > + } else { > + size = sizeof(JSObject); fitzgen: TypedArrayObjects are NativeObjects. (Someday, perhaps they can become typed objects.) The non-NativeObjects I see are: AtomicsObject, SIMDObject, ArrayMetaTypeDescr, StructMetaTypeDescr, TypedObject, ArrayIteratorObject, StringIteratorObject, ArrayBufferViewObject, ProxyObject, StopIterationObject, and UnboxedPlainObject. I'm not really sure which of these will be handled by sizeof(JSObject). It seems like there should be something closer to js::gc::Arena::thingSize(asTenured().getAllocKind()) to default to here? @@ +4112,5 @@ > + return obj.sizeOfIncludingThisInNursery(); > + > + JS::ClassInfo info; > + obj.addSizeOfExcludingThis(mallocSizeOf, &info); > + return obj.asTenured().arenaHeader()->getThingSize() + info.sizeOfAllThings(); This looks to be the same as return obj.tenuredSizeOfThis() + info.sizeOfAllThings();

Comment on attachment 8561257 [details] [diff] [review] Implement JS::ubi::Node::size for JSObjects. Review of attachment 8561257 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/heap-analysis/byteSize-of-object.js @@ +3,5 @@ > +// We actually hard-code specific sizes into this test, even though they're > +// implementation details, because in practice there are only two architecture > +// variants to consider (32-bit and 64-bit), and if these sizes change, that's > +// something SpiderMonkey hackers really want to know; they're supposed to be > +// stable. I think this is actually pretty reasonable. On one hand, it increases the overhead of making changes that shrink objects. On the other hand, it increases the overhead of changes that increase object sizes. So, I think on balance with the frequency we make such changes (e.g. ~never) that this is acceptable to good. @@ +22,5 @@ > + var postGCSize = byteSize(obj); > + if (preGCSize == postGCSize) > + return preGCSize; > + return 0; > +} Tenuring happens to keep the objects size identical because we are overly cautious and not because we should. We've had bugs on file for ages to investigating compacting what we can when tenuring, e.g. bug 874151. I think we'd only want the sizes here to reflect tenured sizes, which they already do. @@ +49,5 @@ > +assertEq(tByteSize({ w:1, x:2, y:3, 0:0, 1:1, 2:2 }), s(148, 168)); > +assertEq(tByteSize({ w:1, x:2, y:3, 0:0, 1:1, 2:2, 3:3, 4:4 }), s(148, 168)); > +assertEq(tByteSize({ w:1, x:2, y:3, z:4, a:6, 0:0 }), s(148, 168)); > +assertEq(tByteSize({ w:1, x:2, y:3, z:4, a:6, 0:0, 1:1, 2:2 }), s(148, 168)); > +assertEq(tByteSize({ w:1, x:2, y:3, z:4, a:6, 0:0, 1:1, 2:2, 3:3, 4:4 }), s(180,200)); Missing space after ,. ::: js/src/jsobj.cpp @@ +4094,5 @@ > + if (native.hasDynamicElements()) { > + js::ObjectElements &elements = *native.getElementsHeader(); > + if (!elements.isCopyOnWrite() || elements.ownerObject() == this) > + size += elements.capacity * sizeof(HeapSlot); > + } If some objects with that sort of constraint do get nursery allocated, they totally wouldn't work. Given that things do more or less work, I expect those object's ::create function requests TenuredHeap when making the GC allocation.

(In reply to Jan de Mooij [:jandem] from comment #7) > Drive-by comment: typed objects and unboxed objects are non-native objects > bigger than this. Especially the latter will become quite popular in normal > JS code when we enable them by default. See UnboxedPlainObject in > vm/UnboxedObject.h. Thanks for pointing those cases out. At the moment, the about:memory collection code doesn't seem to worry about those types, and about:memory is our lodestar at the moment. However, I've filed bug 1133592 and bug 1133593 as follow-ups.

Fresh try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c6150b7d01c

(In reply to Steve Fink [:sfink, :s:] from comment #9) > Would it be too much to ask for this to be moved to getBuildConfiguration() > instead? I'm getting overwhelmed by the number of shell functions these days. Oh, I didn't know about that! Changed.

(In reply to Steve Fink [:sfink, :s:] from comment #10) > ::: js/src/jit-test/tests/heap-analysis/byteSize-of-object.js > @@ +3,5 @@ > > +// We actually hard-code specific sizes into this test, even though they're > > +// implementation details, because in practice there are only two architecture > > +// variants to consider (32-bit and 64-bit), and if these sizes change, that's > > +// something SpiderMonkey hackers really want to know; they're supposed to be > > +// stable. > > Are they? This test is terrifying. Will this survive unboxed objects? Almost certainly not. But the tests pass now, even with --unboxed-objects, so we're evidently not encountering any unboxed objects. If they do show up, then we'll split the tests into boxed and unboxed versions. > @@ +22,5 @@ > > + var postGCSize = byteSize(obj); > > + if (preGCSize == postGCSize) > > + return preGCSize; > > + return 0; > > +} > > Sorry, what is this doing? Does the t in the name mean tenuredByteSize? (And > if so, why return preGCSize instead of postGCSize? Not that it matters given > that they're the same, but if this *is* supposed to stand for > tenuredByteSize, it seems weird.) It's checking that the size of the object doesn't change when it's tenured (if it was indeed allocated in the nursery at all). I'll comment the function. > It seems like you're making this fail if tenuring changes an object's size. > Is that right, and if so, why? If tenuring affects an object's size, then we want to try to test the pre- and post-tenure sizes separately. See the case for the [] literal. > In general, considering the craziness of what this test is, I would much > prefer jimb-style comments. This section should say it's testing objects > with only named properties. > > Why is an empty object larger than one with a single field? That's your guys' problem, not mine! (Assuming the size is *correct*...) > Would it be worth adding a bunch of things like const FINALIZE_OBJECT2 = > s(n32, 64); at the beginning and using those in the asserts? (Or a made-up > SIZE_OBJECT2) That would help explain why 3 properties use the same size as > 4 properties. > > @@ +40,5 @@ > > +assertEq(tByteSize({ 0:0, 1:1, 2:2, 3:3, 4:4 }), s(148, 168)); > > + > > +// Mix indexed and named properties, exploring each combination of the size > > +// classes above. Oddly, the changes here as the objects grow are not simply the > > +// sums of the changes above. > > Perhaps this craziness prevents the SIZE_OBJECT2 stuff. The slot and element arrays don't live in the arena, so I would expect that the allocation kinds are only a base for the total size. > I wonder if this can fail in any of the zeal builds if you happen to get a > GC at just the wrong time. That's a good question; I'll try it out. > ::: js/src/jsobj.cpp > @@ +4094,5 @@ > > + if (native.hasDynamicElements()) { > > + js::ObjectElements &elements = *native.getElementsHeader(); > > + if (!elements.isCopyOnWrite() || elements.ownerObject() == this) > > + size += elements.capacity * sizeof(HeapSlot); > > + } > > This seems good for PlainObjects. I'm not convinced this is adequate for > other NativeObjects. ArrayBufferObjects will never be in the nursery, so > they're ok. TypedArrayObjects with inlined data (and therefore lazy buffers) > have some data after the slots & elements. I honestly don't know how they're > handled by the nursery. I think I'll have to 302 terrence for that. Our guiding light in this process is about:memory. Rather than aiming for complete coverage of every data structure in Firefox, they use DMD to identify sources of significant memory usage that are not covered by the code they have now, and concentrate their efforts on the types that actually use a lot of memory. I think devtools needs to follow the same strategy, and skip types that aren't major contributors yet. If JSObject::sizeOfIncludingThisInNursery gets out of sync with about:memory's JSObject::addSizeOfExcludingThis, which is validated by DMD, then we'll get test suite failures from the pre-tenure vs. post-tenure check. > It seems like there should be something closer to > js::gc::Arena::thingSize(asTenured().getAllocKind()) to default to here? I couldn't find such a thing. > This looks to be the same as > > return obj.tenuredSizeOfThis() + info.sizeOfAllThings(); Fixed --- thanks!

(In reply to Terrence Cole [:terrence] from comment #11) > ::: js/src/jsobj.cpp > @@ +4094,5 @@ > > + if (native.hasDynamicElements()) { > > + js::ObjectElements &elements = *native.getElementsHeader(); > > + if (!elements.isCopyOnWrite() || elements.ownerObject() == this) > > + size += elements.capacity * sizeof(HeapSlot); > > + } > > If some objects with that sort of constraint do get nursery allocated, they > totally wouldn't work. Given that things do more or less work, I expect > those object's ::create function requests TenuredHeap when making the GC > allocation. I'm not sure what you mean here. Are you saying that objects with dynamic elements can never be allocated in the nursery? Or that they can't possibly share their elements, so there's no need to check for copy-on-write and ownership?

Ah, sorry, The interface to splinter here is attrocious: that comment was actually in response to sfink's comment on the same section of code. That, of course, does not carry through to the actual comment placed in the bug, however. The series should read: > > ::: js/src/jsobj.cpp > > @@ +4094,5 @@ > > > + if (native.hasDynamicElements()) { > > > + js::ObjectElements &elements = *native.getElementsHeader(); > > > + if (!elements.isCopyOnWrite() || elements.ownerObject() == this) > > > + size += elements.capacity * sizeof(HeapSlot); > > > + } > > > > This seems good for PlainObjects. I'm not convinced this is adequate for > > other NativeObjects. ArrayBufferObjects will never be in the nursery, so > > they're ok. TypedArrayObjects with inlined data (and therefore lazy buffers) > > have some data after the slots & elements. I honestly don't know how they're > > handled by the nursery. I think I'll have to 302 terrence for that. > > If some objects with that sort of constraint do get nursery allocated, they > totally wouldn't work. Given that things do more or less work, I expect > those object's ::create function requests TenuredHeap when making the GC > allocation.

This isn't meant to change any of the actual behavior, just move the code around.

(Carrying forward review.) This version uses the allocKindForTenure method of JSObject added in the earlier patch to get a better base size for objects in the nursery. With this, we can remove the special case in the test for empty arrays, as they no longer seem to change size when tenured (they don't; we just weren't getting the right size for them in the nursery). This in turn means that the tests pass under all GC zeal settings.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7681bcbb7a87

Comment on attachment 8576339 [details] [diff] [review] Make JSObject::allocKindForTenure out of GetObjectAllocKindForCopy. Review of attachment 8576339 [details] [diff] [review]: ----------------------------------------------------------------- Please double check that this doesn't regress octane performance before landing -- we've had trouble with out-of-lining things in the area before.

Comment on attachment 8581456 [details] [diff] [review] Use mozalloc's moz_malloc_size_of in the JS shell, not our own custom copy. Review of attachment 8581456 [details] [diff] [review]: ----------------------------------------------------------------- ::: moz.build @@ +53,2 @@ > 'memory/volatile', > ] I think it's time to say that we'd be better off with DIRS += ['memory'] in top-level moz.build, and having all the logic of adding fallible, mozalloc, and volatile in there.

(In reply to Terrence Cole [:terrence] from comment #22) > Please double check that this doesn't regress octane performance before > landing -- we've had trouble with out-of-lining things in the area before. Okay, I did some Octane runs. It doesn't seem to make much difference. All --optimize builds, two Octane runs each: changeset 05c0b5cfe341 (inbound 2015-4-2): 30887, 30389 changeset 05c0b5cfe341 + patch stack up to but excluding JSObject::allocKindForTenure: 30976, 30132 changeset 05c0b5cfe341 + patch stack up to and including JSObject::allocKindForTenure: 30796, 30967

Is this what you had in mind?

(In reply to Jim Blandy :jimb from comment #25) > (In reply to Terrence Cole [:terrence] from comment #22) > > Please double check that this doesn't regress octane performance before > > landing -- we've had trouble with out-of-lining things in the area before. > > Okay, I did some Octane runs. It doesn't seem to make much difference. > > All --optimize builds, two Octane runs each: > > changeset 05c0b5cfe341 (inbound 2015-4-2): 30887, 30389 > changeset 05c0b5cfe341 + patch stack up to but excluding > JSObject::allocKindForTenure: 30976, 30132 > changeset 05c0b5cfe341 + patch stack up to and including > JSObject::allocKindForTenure: 30796, 30967 Great! Thanks for checking that!

Comment on attachment 8587801 [details] [diff] [review] Gather up memory subdirectory build decision code into memory/moz.build. Review of attachment 8587801 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/moz.build @@ +22,5 @@ > + if not CONFIG['MOZ_NATIVE_JEMALLOC']: > + DIRS += ['jemalloc'] > + > + if CONFIG['MOZ_REPLACE_MALLOC']: > + DIRS += ['replace'] I think some of those branches are weird, but they do match the current status quo afaics, so I won't bother you ::: moz.build @@ +39,5 @@ > 'mfbt', > 'mozglue', > ] > > + if not CONFIG['JS_STANDALONE']: You can remove this condition, MOZ_WIDGET_TOOLKIT is never going to be "android" for a standalone js build :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fcb10ac4ee5 https://hg.mozilla.org/integration/mozilla-inbound/rev/2be405299a3a https://hg.mozilla.org/integration/mozilla-inbound/rev/c74b408461c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/da10572321fc https://hg.mozilla.org/integration/mozilla-inbound/rev/23a91d7d5f00

Here is a clean try push for that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e827e9fa5d0

(In reply to Pulsebot from comment #29) > https://hg.mozilla.org/integration/mozilla-inbound/rev/8fcb10ac4ee5 This added #include "mozilla/mozalloc.h" to shell/js.cpp, which busted my local builds (using clang 3.7 and 3.6), with these errors: { 0:39.73 In file included from $SRC/js/src/shell/js.cpp:13: 0:39.73 ../../../dist/include/mozilla/mozalloc.h:182:1: error: replacement function 'operator new' cannot be declared 'inline' [-Werror,-Winline-new-delete] 0:39.73 MOZALLOC_INLINE 0:39.73 ^ 0:39.73 ../../../dist/include/mozilla/mozalloc.h:31:27: note: expanded from macro 'MOZALLOC_INLINE' 0:39.73 # define MOZALLOC_INLINE MOZ_ALWAYS_INLINE_EVEN_DEBUG 0:39.73 ^ 0:39.73 ../../../dist/include/mozilla/Attributes.h:27:75: note: expanded from macro 'MOZ_ALWAYS_INLINE_EVEN_DEBUG' 0:39.74 # define MOZ_ALWAYS_INLINE_EVEN_DEBUG __attribute__((always_inline)) inline 0:39.74 ^ } Noting here first; happy to file a followup if it'd be better to deal with it elsewhere.

The flagged line of source code -- js.cpp:13 -- is the new #include for mozalloc.h, BTW.

I spun off bug 1155393 for the issues in comment 31, since they seem to be more of a bug in mozalloc.h than in js.cpp.

https://hg.mozilla.org/mozilla-central/rev/8fcb10ac4ee5 https://hg.mozilla.org/mozilla-central/rev/2be405299a3a https://hg.mozilla.org/mozilla-central/rev/c74b408461c2 https://hg.mozilla.org/mozilla-central/rev/da10572321fc https://hg.mozilla.org/mozilla-central/rev/23a91d7d5f00