Bug 74099 – There should be a way to count the number of nodes held by undo stack

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

To accurately measure and analyze the memory impact of undo stack, we should add some mechanism to count the numer of nodes (+ document) held by undo stack in debug builds.

[Ryosuke Niwa]

Created attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand

[Early Warning System Bot]

Comment on attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand

Attachment 118453 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10807356

[Gyuyoung Kim]

Comment on attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand

Attachment 118453 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10807369

[Ryosuke Niwa]

Comment on attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand

View in context: https://bugs.webkit.org/attachment.cgi?id=118453&action=review

> Source/WebCore/editing/EditCommand.cpp:258
> +void SimpleEditCommand::addNodeAndDescedents(Node* startNode, HashSet<Node*>& nodes)
> +{
> +    for (Node* node = startNode; node; node = node->traverseNextNode(startNode))
> +        nodes.add(node);
> +}

Oops I need to wrap this in ifndef. Will do once this patch is reviewed.

[Gustavo Noronha (kov)]

Comment on attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand

Attachment 118453 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10809390

[Darin Adler]

Comment on attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand

View in context: https://bugs.webkit.org/attachment.cgi?id=118453&action=review

Can we come up with some kind of trick to do this during doApply and use a global variable so we don’t have to add all these ifdefs and all these additional functions?

> Source/WebCore/editing/EditCommand.h:111
> +    void addNodeAndDescedents(Node*, HashSet<Node*>&);

Spelling mistake here. The word is “descendants”.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 118453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118453&action=review
> 
> Can we come up with some kind of trick to do this during doApply and use a global variable so we don’t have to add all these ifdefs and all these additional functions?

Yeah, I've tried to come with a way but it's hard to override an assignment to RefPtr... I mean I could wrap RefPtr<> but that seemed like a too much trouble.

> > Source/WebCore/editing/EditCommand.h:111
> > +    void addNodeAndDescedents(Node*, HashSet<Node*>&);
> 
> Spelling mistake here. The word is “descendants”.

Oops. Will fix.

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #6)
> > Can we come up with some kind of trick to do this during doApply and use a global variable so we don’t have to add all these ifdefs and all these additional functions?
> 
> Yeah, I've tried to come with a way but it's hard to override an assignment to RefPtr... I mean I could wrap RefPtr<> but that seemed like a too much trouble.

I think maybe you’re misunderstanding my suggestion.

I just meant that we’d add code at the end of each doApply that would add the nodes to a global collection.

[Ryosuke Niwa]

(In reply to comment #8)
> I think maybe you’re misunderstanding my suggestion.
> 
> I just meant that we’d add code at the end of each doApply that would add the nodes to a global collection.

Oh I see. But we might over count if we do that since some ports limit the depth of undo stack.

[Enrica Casucci]

Comment on attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand

Looks good to me with the spelling correction Darin pointed out.

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #8)
> > I think maybe you’re misunderstanding my suggestion.
> > 
> > I just meant that we’d add code at the end of each doApply that would add the nodes to a global collection.
> 
> Oh I see. But we might over count if we do that since some ports limit the depth of undo stack.

Sure, but I was thinking the housekeeping would be done by some code managing the edit commands to try to associate the nodes with the right commands. But without having to add a function to every edit command class.

[Ryosuke Niwa]

(In reply to comment #11)
> Sure, but I was thinking the housekeeping would be done by some code managing the edit commands to try to associate the nodes with the right commands. But without having to add a function to every edit command class.

I don't think I'm following you here. The interesting data is the number of nodes kept alive in the undo stack, and to figure that out, we'd have to call getNodesInCommand on all editing commands that are in the undo stack. e.g. if same nodes are kept by multiple editing commands, then we don't want to count them twice. Similarly, if some editing commands go way, then we'd have to recompute the number.

[Ryosuke Niwa]

An alternative approach we can take is to use HashMap<Node*, unsigned> where the second value indicates the number of edit commands that holds onto this node. We can then decrement the counter in the destructor of simple edit commands.

[WebKit Review Bot]

Comment on attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand

Attachment 118453 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10809454

[Ryosuke Niwa]

Anyway, I think this is a valuable patch as is. We can always improve or even revert this and implement another approach once we find a better way.

[Ryosuke Niwa]

Committed r102527: <http://trac.webkit.org/changeset/102527>

[Ryosuke Niwa]

Build fixes in http://trac.webkit.org/changeset/102529 and http://trac.webkit.org/changeset/102531