Bug 74099

Summary: There should be a way to count the number of nodes held by undo stack
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, enrica, gregsimon, gustavo, ojan, sullivan, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 74059    
Attachments:
Description Flags
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand enrica: review+, webkit-ews: commit-queue-

Description Ryosuke Niwa 2011-12-08 09:46:44 PST
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.
Comment 1 Ryosuke Niwa 2011-12-08 12:49:33 PST
Created attachment 118453 [details]
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand
Comment 2 Early Warning System Bot 2011-12-08 13:02:58 PST
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
Comment 3 Gyuyoung Kim 2011-12-08 13:41:10 PST
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
Comment 4 Ryosuke Niwa 2011-12-08 13:42:17 PST
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.
Comment 5 Gustavo Noronha (kov) 2011-12-08 13:47:10 PST
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
Comment 6 Darin Adler 2011-12-08 14:34:05 PST
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”.
Comment 7 Ryosuke Niwa 2011-12-08 14:53:22 PST
(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.
Comment 8 Darin Adler 2011-12-08 14:55:34 PST
(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.
Comment 9 Ryosuke Niwa 2011-12-08 15:05:07 PST
(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.
Comment 10 Enrica Casucci 2011-12-08 16:15:52 PST
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.
Comment 11 Darin Adler 2011-12-08 16:21:29 PST
(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.
Comment 12 Ryosuke Niwa 2011-12-08 16:25:00 PST
(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.
Comment 13 Ryosuke Niwa 2011-12-08 16:26:59 PST
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.
Comment 14 WebKit Review Bot 2011-12-08 16:57:36 PST
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
Comment 15 Ryosuke Niwa 2011-12-09 13:19:26 PST
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.
Comment 16 Ryosuke Niwa 2011-12-10 13:35:13 PST
Committed r102527: <http://trac.webkit.org/changeset/102527>
Comment 17 Ryosuke Niwa 2011-12-10 15:46:42 PST
Build fixes in http://trac.webkit.org/changeset/102529 and http://trac.webkit.org/changeset/102531