1194422 - Move census traversal to js/public/UbiNodeCensus.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This is required so that embedders can do census traversals on offline heap snapshots.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Expose census traversals to SpiderMonkey embedders

This moves census types and functions to js/public/UbiNodeCensus.h and
js/src/vm/UbiNodeCensus.cpp. This is required so that embedders can do census
traversals on offline heap snapshots.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8647692 [details] [diff] [review]
Expose census traversals to SpiderMonkey embedders

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

Ok, I didn't realize this was just code motion, so I'll mark this r+ but I'd be interested in hearing the answers to my questions below.

::: js/public/UbiNodeCensus.h
@@ +11,5 @@
> +
> +#include "js/UbiNode.h"
> +#include "js/UbiNodeTraverse.h"
> +
> +// A census is a ubi::Node traversal that counts each node into various buckets.

I'm really just being anal here, since I fully understand what you mean here. But "count...into" sounds weird, and the way you put *a* node into *various* buckets does raise the question of whether a node goes into one bucket or possibly multiple. "...traversal that assigns each node to one or more buckets, and returns the size of each bucket"? Kinda wordy.

Wait, let me read on and see what it's actually doing...

@@ +28,5 @@
> +//       - nodes with a specific ubi::Node::typeName *
> +//
> +// Obviously, the parts of this tree marked with * represent many separate
> +// counts, depending on how many distinct [[Class]] values and ubi::Node type
> +// names we encounter.

Maybe instead of using '*', say "- objects grouped by [[Class]]"? And perhaps, eliminate that level of the hierarchy:

//   - all nodes
//     - objects, grouped by [[Class]]
//     - strings
//     - scripts
//     - all other Node types, grouped by ubi::Node::typeName

(after reading further: no, it looks like this matches the CountType hierarchy, so you probably do want these here. But that's hard to figure out without at least a forward pointer to CountType.]

@@ +36,5 @@
> +
> +namespace JS {
> +namespace ubi {
> +
> +// Common data for a census traversal, shared across all CountType nodes.

CountType? Wazzat? AFAICT, CountType's definition would be ok with a Census forward decl. It seems like this would be easier to read if you swapped the declaration order.

@@ +38,5 @@
> +namespace ubi {
> +
> +// Common data for a census traversal, shared across all CountType nodes.
> +struct Census {
> +    JSContext* const cx;

Is cx only used for OOM reporting? It's a little surprising to see it here.

@@ +40,5 @@
> +// Common data for a census traversal, shared across all CountType nodes.
> +struct Census {
> +    JSContext* const cx;
> +    // If the debuggeeZones set is empty, then all zones are considered.
> +    JS::ZoneSet debuggeeZones;

This is confusing. The comment implies that debuggeeZones is used for a filter predicate. cx clearly is not. What about atomsZone? I think the filters should be separated from the census state.

Also, I'm wondering if the name "debuggeeZones" is naming based on implementation detail. What about "keepZones" or "includedZones" or "filterZones"?

@@ +74,5 @@
> +};
> +
> +using CountBasePtr = UniquePtr<CountBase, CountDeleter>;
> +
> +// Abstract base class for CountType nodes.

I still want to know what a CountType node is. Please expand this comment a little.

@@ +108,5 @@
> +class CountBase {
> +    // In lieu of a vtable, each CountBase points to its type, which
> +    // carries not only the implementations of the CountBase methods, but also
> +    // additional parameters for the type's behavior, as specified in the
> +    // breakdown argument passed to takeCensus.

I was expecting an explanation of why you're not just using vtables and the class hierarchy here, but this comment doesn't cover it. A subclass can obviously store "additional parameters for the type's behavior". What am I missing?

@@ +125,5 @@
> +    // count, and store it in |report|. Return true on success, or false on
> +    // failure.
> +    bool report(MutableHandleValue report) { return type.report(*this, report); }
> +
> +    // Down-cast this CountBase to its true type, based on its type, and run

Maybe "...based on its 'type'", or maybe "...based on its type field"? Reads kinda funny otherwise. Or maybe rename it to type_.

::: js/src/vm/UbiNodeCensus.cpp
@@ +44,5 @@
> +//      other: ByUbinodeType
> +//        each type: SimpleCount
> +//
> +// The interior nodes are all breakdown types that categorize nodes according to
> +// one characteristics or another; and the leaf nodes are all SimpleType.

*characteristic

@@ +55,5 @@
> +// To keep actual count nodes small, they have no vtable. Instead, each count
> +// points to its CountType, which knows how to carry out all the operations we
> +// need on a Count. A CountType can produce new count nodes; process nodes as we
> +// visit them; build a JS object reporting the results; and destruct count
> +// nodes.

Ah! So the vtable explanation is here. In fact, the CountType explanation is too. I think the header file needs either a forward pointer to this documentation, or for these docs to be moved there.

So... why do these count nodes need to be *so* small? I mean, a vtable adds one word to the size, and these are already a couple of words, aren't they? And we're only talking about tens to hundreds here, right?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Steve Fink [:sfink, :s:] from comment #2)
> Comment on attachment 8647692 [details] [diff] [review]
> Expose census traversals to SpiderMonkey embedders
> 
> Review of attachment 8647692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, I didn't realize this was just code motion, so I'll mark this r+ but I'd
> be interested in hearing the answers to my questions below.

Ah, my bad, I should have explained that better.

Most of this code was written by jimb, but I reviewed it and so I *should* be
able to answer ;)

> @@ +36,5 @@
> > +
> > +namespace JS {
> > +namespace ubi {
> > +
> > +// Common data for a census traversal, shared across all CountType nodes.
> 
> CountType? Wazzat? AFAICT, CountType's definition would be ok with a Census
> forward decl. It seems like this would be easier to read if you swapped the
> declaration order.

Good idea re: the forward declaration.

As mentioned below I tried to split a bunch of documentation into implementation
details versus interface and it seems like it might be better to keep it
together, based on your feedback.

> @@ +38,5 @@
> > +namespace ubi {
> > +
> > +// Common data for a census traversal, shared across all CountType nodes.
> > +struct Census {
> > +    JSContext* const cx;
> 
> Is cx only used for OOM reporting? It's a little surprising to see it here.

It is also used for rooting in the various `report` methods.

> @@ +40,5 @@
> > +// Common data for a census traversal, shared across all CountType nodes.
> > +struct Census {
> > +    JSContext* const cx;
> > +    // If the debuggeeZones set is empty, then all zones are considered.
> > +    JS::ZoneSet debuggeeZones;
> 
> This is confusing. The comment implies that debuggeeZones is used for a
> filter predicate. cx clearly is not. What about atomsZone? I think the
> filters should be separated from the census state.
> 
> Also, I'm wondering if the name "debuggeeZones" is naming based on
> implementation detail. What about "keepZones" or "includedZones" or
> "filterZones"?

Yes, it would be nice to split this out into a proper filtering predicate that
gets passed into the `Census`. I'll leave it for a follow-up if you don't mind.

Will do the renaming of `debuggeeZones` here, though. How do you like
`targetZones`?

> @@ +55,5 @@
> > +// To keep actual count nodes small, they have no vtable. Instead, each count
> > +// points to its CountType, which knows how to carry out all the operations we
> > +// need on a Count. A CountType can produce new count nodes; process nodes as we
> > +// visit them; build a JS object reporting the results; and destruct count
> > +// nodes.
> 
> Ah! So the vtable explanation is here. In fact, the CountType explanation is
> too. I think the header file needs either a forward pointer to this
> documentation, or for these docs to be moved there.

Yeah so that did used to be there and I tried to split out "implementation
details" from the public header, but clearly I did a poor job. I'll just move
this all back together in the header.

> So... why do these count nodes need to be *so* small? I mean, a vtable adds
> one word to the size, and these are already a couple of words, aren't they?
> And we're only talking about tens to hundreds here, right?

Yeah I would be surprised if we ever have more than twenty... I should have
asked this question when I reviewed this code :-/

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Expose census traversals to SpiderMonkey embedders

This moves census types and functions to js/public/UbiNodeCensus.h and
js/src/vm/UbiNodeCensus.cpp. This is required so that embedders can do census
traversals on offline heap snapshots.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 6

[Steve Fink [:sfink] [:s:]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #3)
> (In reply to Steve Fink [:sfink, :s:] from comment #2)
> > @@ +36,5 @@
> > > +
> > > +namespace JS {
> > > +namespace ubi {
> > > +
> > > +// Common data for a census traversal, shared across all CountType nodes.
> > 
> > CountType? Wazzat? AFAICT, CountType's definition would be ok with a Census
> > forward decl. It seems like this would be easier to read if you swapped the
> > declaration order.
> 
> Good idea re: the forward declaration.
> 
> As mentioned below I tried to split a bunch of documentation into
> implementation
> details versus interface and it seems like it might be better to keep it
> together, based on your feedback.

I'm ambivalent, actually. I think the division wasn't quite right here, since I didn't feel like I knew enough when reading the interface to know how to use it, but at the same time some of it *did* feel like implementation internals that didn't belong in the interface. It's nice to have someone in the know (the author) separate out the stuff I don't need to concern myself with.

If it's easiest to just move it all back to the .h file, I'm fine with that.

> > @@ +40,5 @@
> > > +// Common data for a census traversal, shared across all CountType nodes.
> > > +struct Census {
> > > +    JSContext* const cx;
> > > +    // If the debuggeeZones set is empty, then all zones are considered.
> > > +    JS::ZoneSet debuggeeZones;
> > 
> > This is confusing. The comment implies that debuggeeZones is used for a
> > filter predicate. cx clearly is not. What about atomsZone? I think the
> > filters should be separated from the census state.
> > 
> > Also, I'm wondering if the name "debuggeeZones" is naming based on
> > implementation detail. What about "keepZones" or "includedZones" or
> > "filterZones"?
> 
> Yes, it would be nice to split this out into a proper filtering predicate
> that
> gets passed into the `Census`. I'll leave it for a follow-up if you don't
> mind.
> 
> Will do the renaming of `debuggeeZones` here, though. How do you like
> `targetZones`?

Works for me, thanks.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f786a3ae0410

Comment 8

[Nathan Froyd [:froydnj]]

Sorry, I had to back this out along with bug 1194418.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c580b5229b

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Expose census traversals to SpiderMonkey embedders

This moves census types and functions to js/public/UbiNodeCensus.h and
js/src/vm/UbiNodeCensus.cpp. This is required so that embedders can do census
traversals on offline heap snapshots.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Moved a couple method definitions from the new header to cpp files. Interesting that this didn't show up in previous try runs.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66939d410cc6

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62e4da484948

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62e4da484948