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

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(5 attachments, 12 obsolete attachments)

11.06 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
15.54 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
20.24 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
1.36 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
7.15 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8653129 - Flags: review?(sphink)
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.
Attachment #8653130 - Flags: review?(sphink)
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Attachment #8653129 - Flags: review?(sphink) → review+
Attachment #8653130 - Flags: review?(sphink) → review+
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.
Attachment #8653131 - Flags: review?(sphink) → review+
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/
Attachment #8653132 - Flags: review?(sphink) → review+
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
Attachment #8653133 - Flags: review?(sphink) → review+
(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.
Attachment #8653131 - Attachment is obsolete: true
Attachment #8653195 - Flags: review+
Attachment #8653132 - Attachment is obsolete: true
Attachment #8653196 - Flags: review+
Depends on: 1196461
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
Rebasing this on top of master so it can land without bug 1196461.
No longer depends on: 1196461
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.
Attachment #8653129 - Attachment is obsolete: true
Attachment #8653529 - Flags: review+
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.
Attachment #8653130 - Attachment is obsolete: true
Attachment #8653530 - Flags: review+
Attachment #8653195 - Attachment is obsolete: true
Attachment #8653531 - Flags: review+
Attachment #8653196 - Attachment is obsolete: true
Attachment #8653533 - Flags: review+
Attachment #8653133 - Attachment is obsolete: true
Attachment #8653534 - Flags: review+
Those are the rebased versions. ni?me as a reminder to do a try push once the trees open again.
Flags: needinfo?(nfitzgerald)
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.
Attachment #8653529 - Attachment is obsolete: true
Attachment #8653689 - Flags: review+
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.
Attachment #8653530 - Attachment is obsolete: true
Attachment #8653691 - Flags: review+
That was a bad rebase. Should be fixed now.

Yet Another Try Push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdc44f8c83dc
You need to log in before you can comment on or make changes to this bug.