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)

x86
macOS
defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(2 files)

> ::: 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: nobody → nfitzgerald
Attachment #8615464 - Flags: review?(jimb)
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+
Attachment #8615722 - Flags: review?(jimb)
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 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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: