1196634 - The ByCoarseType ubi::Node count type should use an enum instead of checking various is<T>

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The is<T>() stuff is a mess for embedders who don't even know about all the T types.

We should define a JS::ubi::Node::coarseType method that returns one variant of an CoarseType enum instead.

Comment 1

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

Attachment: Part 0: Define a JS::ubi::CoarseType enum

This commit defines the JS::ubi::CoarseType enum and a corresponding method
coarseType() on JS::ubi::Node base and concrete classes. It implements the
overrides for all concrete specializations whose referent does not belong to the
default Other type.

Comment 2

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

Attachment: Part 1: Extend the protobuf format for coarseType

This commit extends the heap snapshot's CoreDump.proto protobuf message
definition file with a coarseType property on Node messages. This will enable us
to serialize and deserialize JS::ubi::CoarseType information to and from
serialized heap snapshots.

Comment 3

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

Attachment: Part 2: Serialize and deserialize coarseType

Comment 4

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

Attachment: Part 3: Use coarseType() instead of is<T> in census

Comment 5

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

Attachment: Part 4: Remove JS::ubi::Node::getCanonicalTypeName

Comment 6

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

Comment on attachment 8653131 [details] [diff] [review]
Part 2: Serialize and deserialize coarseType

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

::: js/public/UbiNode.h
@@ +463,5 @@
>      String = 2,
> +    Other  = 3,
> +
> +    FIRST  = Object,
> +    LAST   = Other

Hm. Looking at this again, I wonder if Other should be 0 instead of 3, under the assumption that this list will grow so you'll Other mixed in with the "real" types.

Obviously not a big deal, just an idea.

Comment 7

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

Comment on attachment 8653132 [details] [diff] [review]
Part 3: Use coarseType() instead of is<T> in census

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

::: js/src/vm/UbiNodeCensus.cpp
@@ +215,5 @@
>          return count.strings->count(node);
> +      case JS::ubi::CoarseType::Other:
> +        return count.other->count(node);
> +      default:
> +        MOZ_CRASH("bas JS::ubi::CoarseType in JS::ubi::ByCoarseType::count");

s/bas/bad/

Comment 8

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

Comment on attachment 8653133 [details] [diff] [review]
Part 4: Remove JS::ubi::Node::getCanonicalTypeName

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

r=me, with great pleasure

Comment 9

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

(In reply to Steve Fink [:sfink, :s:] from comment #6)
> ::: js/public/UbiNode.h
> @@ +463,5 @@
> >      String = 2,
> > +    Other  = 3,
> > +
> > +    FIRST  = Object,
> > +    LAST   = Other
> 
> Hm. Looking at this again, I wonder if Other should be 0 instead of 3, under
> the assumption that this list will grow so you'll Other mixed in with the
> "real" types.
> 
> Obviously not a big deal, just an idea.

Good idea.

Comment 10

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

Attachment: Part 2: Serialize and deserialize coarseType

Comment 11

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

Attachment: Part 3: Use coarseType() instead of is<T> in census

Comment 12

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

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

Comment 13

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

Fixed the missing header include and static analysis issue in the patch from bug 1196461. Not sure if those crashes were originating from that patch or this one, but I've done a try push with just that patch as well, so we will find out soon.

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

Comment 14

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

Rebasing this on top of master so it can land without bug 1196461.

Comment 15

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

Attachment: Part 0: Define a JS::ubi::CoarseType enum

This commit defines the JS::ubi::CoarseType enum and a corresponding method
coarseType() on JS::ubi::Node base and concrete classes. It implements the
overrides for all concrete specializations whose referent does not belong to the
default Other type.

Comment 16

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

Attachment: Part 1: Extend the protobuf format for coarseType

This commit extends the heap snapshot's CoreDump.proto protobuf message
definition file with a coarseType property on Node messages. This will enable us
to serialize and deserialize JS::ubi::CoarseType information to and from
serialized heap snapshots.

Comment 17

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

Attachment: Part 2: Serialize and deserialize coarseType

Comment 18

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

Attachment: Part 3: Use coarseType() instead of is<T> in census

Comment 19

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

Attachment: Part 4: Remove JS::ubi::Node::getCanonicalTypeName

Comment 20

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

Those are the rebased versions. ni?me as a reminder to do a try push once the trees open again.

Comment 21

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

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

Comment 22

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

Attachment: Part 0: Define a JS::ubi::CoarseType enum

This commit defines the JS::ubi::CoarseType enum and a corresponding method
coarseType() on JS::ubi::Node base and concrete classes. It implements the
overrides for all concrete specializations whose referent does not belong to the
default Other type.

Comment 23

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

Attachment: Part 1: Extend the protobuf format for coarseType

This commit extends the heap snapshot's CoreDump.proto protobuf message
definition file with a coarseType property on Node messages. This will enable us
to serialize and deserialize JS::ubi::CoarseType information to and from
serialized heap snapshots.

Comment 24

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

Attachment: Part 2: Serialize and deserialize coarseType

Comment 25

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

Attachment: Part 3: Use coarseType() instead of is<T> in census

Comment 26

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

Attachment: Part 4: Remove JS::ubi::Node::getCanonicalTypeName

Comment 27

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

That was a bad rebase. Should be fixed now.

Yet Another Try Push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc44f8c83dc

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bdb99e5743
https://hg.mozilla.org/integration/mozilla-inbound/rev/885334756aed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b759269eab
https://hg.mozilla.org/integration/mozilla-inbound/rev/a760d965dc0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/be25e1af79f4

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7bdb99e5743
https://hg.mozilla.org/mozilla-central/rev/885334756aed
https://hg.mozilla.org/mozilla-central/rev/d7b759269eab
https://hg.mozilla.org/mozilla-central/rev/a760d965dc0b
https://hg.mozilla.org/mozilla-central/rev/be25e1af79f4