[jsdbg2] add byte size to tenure promotions log

RESOLVED FIXED in Firefox 42

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments)

We should have the byte size of the newly tenured object in the tenure promotions log.
Status: NEW → ASSIGNED
Created attachment 8632591 [details] [diff] [review]
Part 0: Document the size property of entries in the tenure promotions log
Attachment #8632591 - Flags: review?(sphink)
Created attachment 8632592 [details] [diff] [review]
Part 1: Add the size of the tenured object to the tenure promotions log entry
Attachment #8632592 - Flags: review?(sphink)
Created attachment 8632593 [details] [diff] [review]
Part 2: Test the size property of entries in the tenure promotions log
Attachment #8632593 - Flags: review?(sphink)
Blocks: 961331
Comment on attachment 8632591 [details] [diff] [review]
Part 0: Document the size property of entries in the tenure promotions log

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

::: js/src/doc/Debugger/Debugger.Memory.md
@@ +303,5 @@
>        `[[Class]]` property, for example "Array", "Date", "RegExp", or (most
>        commonly) "Object".
>  
> +    * *byteSize* is the size of the newly tenured object (within the tenured
> +      heap, not the nursery) in bytes.

Maybe point out that this doesn't include any non-GC (malloced) memory that the object might hold onto?
Attachment #8632591 - Flags: review?(sphink) → review+
Comment on attachment 8632592 [details] [diff] [review]
Part 1: Add the size of the tenured object to the tenure promotions log entry

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

Oops. Ignore my comment on the previous patch, since it is incorrect. (It appears that ubi::Node's stuff *does* include malloced memory.) Though that makes me wonder if users of this might be interested in both? (For instance, if you are looking at how many objects you can allocate before filling up the nursery and triggering a minor GC, you don't want malloced memory included.)
Attachment #8632592 - Flags: review?(sphink) → review+
Comment on attachment 8632593 [details] [diff] [review]
Part 2: Test the size property of entries in the tenure promotions log

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

Nice test, though I suppose it doesn't guarantee that all of those things will get nursery-allocated in the first place. Do you want it to?
Attachment #8632593 - Flags: review?(sphink) → review+
I don't think the test needs to guarantee that (and not all of them do, probably). Just wanted a variety of objects to ensure that we can handle them all if/when they do appear in the logs.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea832514f086
https://hg.mozilla.org/mozilla-central/rev/c2853fd10127
https://hg.mozilla.org/mozilla-central/rev/19817a9103a7
https://hg.mozilla.org/mozilla-central/rev/3999da0ba5c9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.