Closed
Bug 814229
Opened 12 years ago
Closed 12 years ago
Measure data hanging off CTypes objects
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [js:t][MemShrink:P2])
Attachments
(3 files)
11.06 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
24.73 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
2.21 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•12 years ago
|
Summary: Measure data hanging of CTypes objects → Measure data hanging off CTypes objects
Assignee | ||
Comment 1•12 years ago
|
||
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.
Attachment #684242 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•12 years ago
|
||
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(-)
Attachment #684243 -
Flags: review?(jorendorff)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [js:t][MemShrink] → [js:t][MemShrink:P2]
Comment 3•12 years ago
|
||
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.
Attachment #684242 -
Flags: review?(jorendorff) → review+
Updated•12 years ago
|
Attachment #684243 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 4•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3fc7f4f20a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/9b58f6e07b21
Comment 5•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f3fc7f4f20a0 https://hg.mozilla.org/mozilla-central/rev/9b58f6e07b21
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
It's fine to not uplift this one. It is probably marginally useful at best.
This broke threadsafe shell builds, with an undefined symbol "js::SizeOfDataIfCDataObject". AWFY can't build anymore.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•12 years ago
|
||
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.
Attachment #694700 -
Flags: review?(dvander)
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!
Attachment #694700 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8429f81df8
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fe8429f81df8
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•