Closed
Bug 1147684
Opened 9 years ago
Closed 9 years ago
Consider making HeapSnapshot::NodeMap store DeserializedNodes directly, rather than UniquePtrs
Categories
(DevTools :: Memory, defect)
Tracking
(firefox41 fixed)
RESOLVED
FIXED
Firefox 41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: fitzgen, Assigned: fitzgen)
References
Details
Attachments
(2 files)
8.13 KB,
patch
|
jimb
:
feedback+
|
Details | Diff | Splinter Review |
6.05 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
> ::: toolkit/devtools/server/HeapSnapshot.h
> @@ +75,5 @@
> > +
> > + // The id of the root node for this deserialized heap graph.
> > + NodeId rootId;
> > +
> > + // The set of nodes in this deserialized heap graph, keyed by id.
>
> Suggestion for the future: I think this could save a lot of memory by
> holding the DeserializedNodes directly:
>
> js::HashMap<NodeId, DeserializedNode>
>
> because the hash table is frozen once 'init' is over, so ubi::Nodes pointing
> directly into the table won't be invalidated by table resizing.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nfitzgerald
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8615464 -
Flags: review?(jimb)
Comment 2•9 years ago
|
||
Comment on attachment 8615464 [details] [diff] [review] HeapSnapshot::NodeMap store DeserializedNodes directly, rather than UniquePtrs Review of attachment 8615464 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but I'd like to see it again with the suggestion below implemented. ::: toolkit/devtools/server/DeserializedNode.cpp @@ +76,5 @@ > + return *this; > +} > + > +bool > +DeserializedNode::init(const protobuf::Node& node, HeapSnapshot& owner) Would it be possible to keep the DeserializedNode constructor that populates all its members with real values, and then have HeapSnapshot::saveNode do all the protobuf message wrangling, and just call that constructor? saveNode is already fallible.
Attachment #8615464 -
Flags: review?(jimb) → feedback+
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f2fdcefa7db
Assignee | ||
Updated•9 years ago
|
Attachment #8615722 -
Flags: review?(jimb)
Assignee | ||
Comment 5•9 years ago
|
||
Note the ordering of my patches (you can see in the try push): Part 1 of this bug -> patch from bug 1148642 -> part 2 from this bug I didn't want to have to rebase my other patches for this change.
Comment 6•9 years ago
|
||
Comment on attachment 8615722 [details] [diff] [review] Part 2: Make the DeserializedNode cosntructor infallible Review of attachment 8615722 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this is much nicer! ::: toolkit/devtools/server/HeapSnapshot.cpp @@ +128,5 @@ > + edges.infallibleAppend(Move(edge)); > + } > + > + DeserializedNode dn(id, typeName, size, Move(edges), *this); > + return nodes.putNew(id, Move(dn)); Note that if you write the constructor directly as the argument to putNew, there's no need for Move.
Attachment #8615722 -
Flags: review?(jimb) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/858d09af88bc https://hg.mozilla.org/integration/mozilla-inbound/rev/c3840ca0384f
https://hg.mozilla.org/mozilla-central/rev/858d09af88bc https://hg.mozilla.org/mozilla-central/rev/c3840ca0384f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•