Last Comment Bug 814229 - Measure data hanging off CTypes objects
: Measure data hanging off CTypes objects
Status: RESOLVED FIXED
[js:t][MemShrink:P2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
: general
Mentors:
Depends on:
Blocks: B2GDarkMatter
  Show dependency treegraph
 
Reported: 2012-11-21 16:01 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-12-22 06:53 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1) - Add "objects-extra/ctypes-data" memory report. (11.06 KB, patch)
2012-11-21 17:07 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
jorendorff: review+
Details | Diff | Review
(part 2) - Refactor storage of the "objects-extra" numbers. (24.73 KB, patch)
2012-11-21 17:08 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
jorendorff: review+
Details | Diff | Review
(part 3) - Unbreak JS shell builds that use --enable-threadsafe. (2.21 KB, patch)
2012-12-20 21:27 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
dvander: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-11-21 16:01:41 PST
Attachment 683892 [details] has stacks indicating that we have ~100KiB of ctypes data that's been created by ConstructData().  This bug is about adding memory reporting for that data.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-11-21 17:07:07 PST
Created attachment 684242 [details] [diff] [review]
(part 1) - Add "objects-extra/ctypes-data" memory report.

This patch adds the "objects-extra/ctypes-data" measurement.
SizeOfDataIfCDataObject() is a bit clumsy but it has the virtue of being the
smallest change to the ctypes interface that I could come up with.
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-11-21 17:08:15 PST
Created attachment 684243 [details] [diff] [review]
(part 2) - Refactor storage of the "objects-extra" numbers.

This patch does some clean-up:

- The six measurements reported under the "objects-extra" sub-tree in
  about:memory are collated into a new struct ObjectsExtraSizes, which is
  similar to the existing TypeInferenceSizes.

- TypeInferenceSizes' constructor is simplified to use memset to zero
  everything.

- CompartmentStats::typeInferenceSizes is renamed as
  CompartmentStats::typeInference, which matches the general rule that names
  of things in CompartmentStats match the strings used in about:memory (i.e.
  "type-inference" in this case).

- TypeObject::sizeOfExcludingThis() is changed to return a value instead of
  updating a field in the TypeInferenceSizes struct.  This makes it like other
  sizeOf*() functions.

diffstat says:

  7 files changed, 86 insertions(+), 112 deletions(-)
Comment 3 Jason Orendorff [:jorendorff] 2012-12-07 10:07:00 PST
Comment on attachment 684242 [details] [diff] [review]
(part 1) - Add "objects-extra/ctypes-data" memory report.

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

::: js/src/ctypes/CTypes.cpp
@@ +1338,5 @@
> +        return 0;
> +
> +    size_t n = 0;
> +    jsval slot = JS_GetReservedSlot(obj, ctypes::SLOT_OWNS);
> +    if (!JSVAL_IS_VOID(slot)) {

Yeah, the style of the surrounding file is pretty crufty. But feel free to use slot.isUndefined() and so forth if you like.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1586,5 @@
> +
> +    CREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("objects-extra/ctypes-data"),
> +                  cStats.objectsExtraCtypesData,
> +                  "Memory allocated for data belonging to ctypes objects.");
> + 

Looks like there's a space character on this blank line.
Comment 6 Justin Lebar (not reading bugmail) 2012-12-20 13:53:56 PST
njn mentioned this bug to me on IRC for potential b2g uplift.

I'm tempted to mostly give up on B2G heap-unclassified until we have reporting for workers with ctypes, since that's upwards of 30% of our dark matter.

Also uplifting these guys is often a pain because last time I checked we haven't uplifted /all/ of our changes to these files.

But let me know if you think this might help and I or someone can do some tests on B2G to see how much memory these cover.
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-12-20 14:00:15 PST
It's fine to not uplift this one.  It is probably marginally useful at best.
Comment 8 David Anderson [:dvander] 2012-12-20 18:38:31 PST
This broke threadsafe shell builds, with an undefined symbol "js::SizeOfDataIfCDataObject". AWFY can't build anymore.
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-12-20 21:27:18 PST
Created attachment 694700 [details] [diff] [review]
(part 3) - Unbreak JS shell builds that use --enable-threadsafe.

Sorry about the bustage.  Here's a fix.  I tested it on:

- A normal shell build.
- An --enable-threadsafe shell build.
- A normal browser build.
Comment 10 David Anderson [:dvander] 2012-12-20 21:29:09 PST
Comment on attachment 694700 [details] [diff] [review]
(part 3) - Unbreak JS shell builds that use --enable-threadsafe.

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

Thanks!
Comment 11 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-12-21 00:04:02 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8429f81df8

Note You need to log in before you can comment on or make changes to this bug.