bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 43

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 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.
Created attachment 8653129 [details] [diff] [review]
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.
Attachment #8653129 - Flags: review?(sphink)
Created attachment 8653130 [details] [diff] [review]
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.
Attachment #8653130 - Flags: review?(sphink)
Created attachment 8653131 [details] [diff] [review]
Part 2: Serialize and deserialize coarseType
Attachment #8653131 - Flags: review?(sphink)
Created attachment 8653132 [details] [diff] [review]
Part 3: Use coarseType() instead of is<T> in census
Attachment #8653132 - Flags: review?(sphink)
Created attachment 8653133 [details] [diff] [review]
Part 4: Remove JS::ubi::Node::getCanonicalTypeName
Attachment #8653133 - 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.
Created attachment 8653195 [details] [diff] [review]
Part 2: Serialize and deserialize coarseType
Attachment #8653131 - Attachment is obsolete: true
Attachment #8653195 - Flags: review+
Created attachment 8653196 [details] [diff] [review]
Part 3: Use coarseType() instead of is<T> in census
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
Created attachment 8653529 [details] [diff] [review]
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.
Attachment #8653129 - Attachment is obsolete: true
Attachment #8653529 - Flags: review+
Created attachment 8653530 [details] [diff] [review]
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.
Attachment #8653130 - Attachment is obsolete: true
Attachment #8653530 - Flags: review+
Created attachment 8653531 [details] [diff] [review]
Part 2: Serialize and deserialize coarseType
Attachment #8653195 - Attachment is obsolete: true
Attachment #8653531 - Flags: review+
Created attachment 8653533 [details] [diff] [review]
Part 3: Use coarseType() instead of is<T> in census
Attachment #8653196 - Attachment is obsolete: true
Attachment #8653533 - Flags: review+
Created attachment 8653534 [details] [diff] [review]
Part 4: Remove JS::ubi::Node::getCanonicalTypeName
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)
Created attachment 8653689 [details] [diff] [review]
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.
Attachment #8653529 - Attachment is obsolete: true
Attachment #8653689 - Flags: review+
Created attachment 8653691 [details] [diff] [review]
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.
Attachment #8653530 - Attachment is obsolete: true
Attachment #8653691 - Flags: review+
Created attachment 8653692 [details] [diff] [review]
Part 2: Serialize and deserialize coarseType
Attachment #8653531 - Attachment is obsolete: true
Attachment #8653692 - Flags: review+
Created attachment 8653693 [details] [diff] [review]
Part 3: Use coarseType() instead of is<T> in census
Attachment #8653533 - Attachment is obsolete: true
Attachment #8653693 - Flags: review+
Created attachment 8653695 [details] [diff] [review]
Part 4: Remove JS::ubi::Node::getCanonicalTypeName
Attachment #8653534 - Attachment is obsolete: true
Attachment #8653695 - Flags: review+
That was a bad rebase. Should be fixed now.

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