Bug 74099 - There should be a way to count the number of nodes held by undo stack
Summary: There should be a way to count the number of nodes held by undo stack
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 74059
  Show dependency treegraph
 
Reported: 2011-12-08 09:46 PST by Ryosuke Niwa
Modified: 2011-12-10 15:46 PST (History)
9 users (show)

See Also:


Attachments
Adds a debug-only member function to EditCommandComposition and SimpleEditCommand (22.40 KB, patch)
2011-12-08 12:49 PST, Ryosuke Niwa
enrica: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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