Closed Bug 1199216 Opened 4 years ago Closed 4 years ago

Implement JS::ubi::Node::size for JSScript referents

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1199215 +++
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Comment on attachment 8662647 [details] [diff] [review]
Implement JS::ubi::Node::size for JSScript referents

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

I think it's totally your judgement call what to include in these sizes, but the things I noticed missing when comparing to JSScript::finalize are
 - script counts map entry
 - debug script
 - lazy script cache entry

If you want to ignore those and land as-is, I'm totally cool with that. (In my quick skimming, I couldn't see a place where about:memory scans those maps separately, but I didn't look that hard.)

::: js/src/builtin/TestingFunctions.cpp
@@ +2477,5 @@
> +
> +    {
> +        // We can't tolerate the GC moving things around while we're using a
> +        // ubi::Node. Check that nothing we do causes a GC.
> +        JS::AutoCheckCannotGC autoCannotGC;

Should we declare JS::ubi::Node to be a GC pointer to the hazard analysis? Then you wouldn't need to do anything here, but I don't know if there are situations where you *do* hold onto a node across a GC.

::: js/src/jit-test/tests/heap-analysis/byteSize-of-scripts.js
@@ +42,5 @@
> +}
> +
> +print("byteSizeOfScript(f2) = " + byteSizeOfScript(f2));
> +assertEq(byteSizeOfScript(f2) > 1, true);
> +assertEq(byteSizeOfScript(f2) > byteSizeOfScript(f1), true);

Heh. Yeah, anything more than this would probably be more annoying than helpful.
Attachment #8662647 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink, :s:] from comment #2)
> ::: js/src/builtin/TestingFunctions.cpp
> @@ +2477,5 @@
> > +
> > +    {
> > +        // We can't tolerate the GC moving things around while we're using a
> > +        // ubi::Node. Check that nothing we do causes a GC.
> > +        JS::AutoCheckCannotGC autoCannotGC;
> 
> Should we declare JS::ubi::Node to be a GC pointer to the hazard analysis?
> Then you wouldn't need to do anything here, but I don't know if there are
> situations where you *do* hold onto a node across a GC.

JS::ubi::Nodes backed by the live heap should be considered gc pointers, but JS::ubi::Nodes backed by offline heap snapshots should not. So far, we have been writing our analyses to work with both kinds, so perhaps it would make sense? Most everything goes through JS::ubi::BreadthFirst, which requires a reference to an AutoCheckCannotGC, so I think we're pretty safe, but yeah it might make sense to make JS::ubi::Node a gc pointer itself.
https://hg.mozilla.org/mozilla-central/rev/e46f4a5bd558
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1211331
You need to log in before you can comment on or make changes to this bug.