Bug 32884

Patch to follow soon.

Created attachment 45474 [details] Proposed Patch

style-queue ran check-webkit-style on attachment 45474 [details] without any errors.

Created attachment 46393 [details] Proposed Patch; A few function name changes and performance improvements over last submission.

Comment on attachment 46393 [details] Proposed Patch; A few function name changes and performance improvements over last submission. > + for (next = first;; next = next->m_nextSibling) { Normally we put a space between the semicolons in lines like this one. > + newChild->m_firstChild = newChild->m_lastChild = 0; Normally we don't merge assignment statements together like this. > newChild->m_countInParent = newChild->computeCountInParent(); > + newChild->resetRenderer(identifier); > + first->recount(identifier); > + return; > } Normally we don't have a return statement at the end of a function like this. > + for (RenderObject* currentRenderer = object->nextInPreOrder(stayWithin); currentRenderer;currentRenderer = currentRenderer->nextInPreOrder(stayWithin)) { Missing space after the semicolon here. > + if (!currentRenderer->m_hasCounterNodeMap || !(currentCounter = maps.get(currentRenderer)->get(identifier.impl()))) > + continue; I think this would be clearer with separate if statements. There's no real reason to have the assignment inside an if statement and having separate ones would make that possible. > + // Do not delete map even if empty as this function should not be called when all counters for a renderer > + // are deleted destroyCounterNodes should be called instead. The map may be empty only temporarely while Typo: "temporarely". Need punctuation between "deleted" and "destroyCounterNodes". Is there a way to do some sort of assertion to check the assumption from this comment? > +void RenderCounter::rendererSubtreeAttached(RenderObject* renderer) > +{ > + ASSERT(renderer); > + RenderObject* descendant = renderer; > + do { > + updateCounters(descendant); > + descendant = descendant->nextInPreOrder(renderer); > + } while (descendant); > +} > } // namespace WebCore At the cost of one extra null check we could write this with a for loop, which would be even clearer and easier to read. Would be good to have a blank line before the end of the namespace. Since this is for the commit-queue, I'll say review- so we can at least fix the typos.

Created attachment 46407 [details] Proposed Patch; Addressed Darin's concerns. Removed an invalid assertion and handled the case. Darin, Unfortunately it is not possible to check the assertion in the comment you referred to above since the map may be empty for brief periods of time, but it should be soon filled. If you want, though, I can remove the comment, delete the map and incur the performance and code size penalty, but I'd like you to consider, first, the following additional information: - in the destructor of RenderObject "destroyCounters" is called, which deletes the map, so as long as this stays true, the map is not leaked, it is just cashed. - this function is only called from within the counters code which is small and should be easy to check for making the correct calls.

(In reply to comment #6) > - this function is only called from within the counters code which is small and > should be easy to check for making the correct calls. Please explain further why it’s easy to check. What would I check?

Comment on attachment 46407 [details] Proposed Patch; Addressed Darin's concerns. Removed an invalid assertion and handled the case. > + CounterNode* currentCounter; > + for (RenderObject* currentRenderer = object->nextInPreOrder(stayWithin); currentRenderer; currentRenderer = currentRenderer->nextInPreOrder(stayWithin)) { > + if (!currentRenderer->m_hasCounterNodeMap) > + continue; > + currentCounter = maps.get(currentRenderer)->get(identifier.impl()); There's no reason for currentCounter to be defined outside the loop. It should be defined right here where it's initialized. > + // Do not delete map even if empty as this function should not be called when all counters for a renderer > + // are deleted destroyCounterNodes should be called instead. The map may be empty only temporarily while > + // old counters associated with a renderer are deleted before new ones are created. There should be some punctuation, maybe a period or semicolon, between the word "deleted" and "destroyCounterNodes". Without a break there, the sentence doesn't make sense. I'd like to see the fast case, no counters involved, in RenderObject::addChild be as simple as possible. At the moment it seems that adding a child always involves walking the child's subtree and does it with multiple levels of function calls. With a bit more inlining the speed impact could be reduced. Or we could make further design changes to make the cost even smaller in such cases. I'm concerned this could have a measurable effect on our core benchmarks, such as the "page load test" used internally at Apple and the i-Bench HTML test. I'm tempted to say review+ but I have two questions still outstanding: 1) Have we done performance tests on non-counter pages to see what the effect of this is? Stephanie Lewis may be able to help you by doing some testing if you don't know a way to do it yourself. 2) There's a separate stayWithin patch. Does this affect the result of the test, or is it a change that affects only performance, not corretness?

(In reply to comment #7) > (In reply to comment #6) > > - this function is only called from within the counters code which is small and > > should be easy to check for making the correct calls. > > Please explain further why it’s easy to check. What would I check? The idea is to grep for the function name and see that it is used only in a few places (for now 1 but 2 more are needed for 11031) and in all cases the counter node objects are destroyed, but the style attribute that drives them stays so the nodes will be recreated at the next layout or earlier.

(In reply to comment #9) > The idea is to grep for the function name and see that it is used only in a few > places (for now 1 but 2 more are needed for 11031) and in all cases the counter > node objects are destroyed, but the style attribute that drives them stays so > the nodes will be recreated at the next layout or earlier. If the comment was made slightly more concrete, then it would make that clear. The comment says something much more oblique. I would say something more like this: // At the time of this writing, all callers are deleting counter nodes that will be re-created shortly. // Specifically, DOM tree and style that caused the counters to be created have not changed. // And worst case, the empty map does little harm and will be deleted when the RenderObject is. The comment above isn't perfect, but there’s a greater chance later contributors will understand it, as opposed to the more abstract remarks in the patch, which are vague enough that they are hard to challenge. By the way, I am not confident that the comment is true, and that counter nodes will always be recreated in such cases. What about things like pseudo styles that can cause the need for counters to change?

(In reply to comment #10) > (In reply to comment #9) > > The idea is to grep for the function name and see that it is used only in a few > > places (for now 1 but 2 more are needed for 11031) and in all cases the counter > > node objects are destroyed, but the style attribute that drives them stays so > > the nodes will be recreated at the next layout or earlier. > > If the comment was made slightly more concrete, then it would make that clear. > The comment says something much more oblique. I would say something more like > this: > > // At the time of this writing, all callers are deleting counter nodes that > will be re-created shortly. > // Specifically, DOM tree and style that caused the counters to be created have > not changed. > // And worst case, the empty map does little harm and will be deleted when the > RenderObject is. > > The comment above isn't perfect, but there’s a greater chance later > contributors will understand it, as opposed to the more abstract remarks in the > patch, which are vague enough that they are hard to challenge. > > By the way, I am not confident that the comment is true, and that counter nodes > will always be recreated in such cases. What about things like pseudo styles > that can cause the need for counters to change? I'll make the changes to the comment as you suggested if you want me to. I can also add in the code necessary to delete the map if it is empty. Either case would not matter much since the function is rarely called unless somebody decides to use webkit as a text editor for a document which is mostly a huge hierarchical table of contents, or something to that effect which results in many counters, especially of the reset kind being deleted and moved and deleted again via complex DOM manipulation. I believe that it is a close call whether to delete the map or not and as a reviewer it is yours to make. I am just providing the information so that you can decide. Upon deeper thinking I guess that one may be able to find a corner case when an empty counter map can survive needlessly until the renderer it is associated with is destroyed.

(In reply to comment #8) > (From update of attachment 46407 [details]) > > + CounterNode* currentCounter; > > + for (RenderObject* currentRenderer = object->nextInPreOrder(stayWithin); currentRenderer; currentRenderer = currentRenderer->nextInPreOrder(stayWithin)) { > > + if (!currentRenderer->m_hasCounterNodeMap) > > + continue; > > + currentCounter = maps.get(currentRenderer)->get(identifier.impl()); > > There's no reason for currentCounter to be defined outside the loop. It should > be defined right here where it's initialized. > > > + // Do not delete map even if empty as this function should not be called when all counters for a renderer > > + // are deleted destroyCounterNodes should be called instead. The map may be empty only temporarily while > > + // old counters associated with a renderer are deleted before new ones are created. > > There should be some punctuation, maybe a period or semicolon, between the word > "deleted" and "destroyCounterNodes". Without a break there, the sentence > doesn't make sense. > > I'd like to see the fast case, no counters involved, in RenderObject::addChild > be as simple as possible. At the moment it seems that adding a child always > involves walking the child's subtree and does it with multiple levels of > function calls. With a bit more inlining the speed impact could be reduced. Or > we could make further design changes to make the cost even smaller in such > cases. > > I'm concerned this could have a measurable effect on our core benchmarks, such > as the "page load test" used internally at Apple and the i-Bench HTML test. > > I'm tempted to say review+ but I have two questions still outstanding: > > 1) Have we done performance tests on non-counter pages to see what the > effect of this is? Stephanie Lewis may be able to help you by doing some > testing if you don't know a way to do it yourself. > > 2) There's a separate stayWithin patch. Does this affect the result of the > test, or is it a change that affects only performance, not corretness? I have run a performance test involving adding and removing many nodes in the most unfavorable position from javascript. collected timings with 5 digits precision. Individual timings would vary both ways so I calculated averages with and without the patch and did statistical analysis on the outputs and I was unable to prove any difference, which is expected since for non-counter nodes, there are only a few jumps and ifs to be compared with the whole effort needed for relayout or compiling and running the JS to drive the creation of the node and the insertion. Yes, the stayWithin patch is expected to improve performance overall and a few of the outcomes for counters. I do not have a test yet where stayWithin should affect other Layout issues.

My primary concern is performance in documents with no counters.

(In reply to comment #13) > My primary concern is performance in documents with no counters. Sorry, if I wasn't clear, my tests involved documents with no counters. For documents with counters, there is obviously a slight impact, but only when nodes with counters on them are added since the counter hierarchy needs to be maintained.

(In reply to comment #14) > (In reply to comment #13) > > My primary concern is performance in documents with no counters. > > Sorry, if I wasn't clear, my tests involved documents with no counters. For > documents with counters, there is obviously a slight impact, but only when > nodes with counters on them are added since the counter hierarchy needs to be > maintained. Adding and removing nodes with JavaScript is probably not the best way to test performance. The normal case of loading and rendering a page is the primary thing we want to test. The RenderObject::addChild function is invoked in that case.

I tested the patch for performance issues on SnowLeopard and didn't come across any problems.

(In reply to comment #16) > I tested the patch for performance issues on SnowLeopard and didn't come across > any problems. Thank you very much, Stephanie, for your help. Darin, About the last pending issue, which is whether or not to delete the counter map in RenderCounter::destroyCounter if it is empty, what is your verdict: A. Should I make a code change to delete the map if empty. B. Should I change the comment to say something like this: // The counterMap is not deleted here even if empty because we expect it to // be reused soon. In order for a renderer to loose all its counters // permanently, the corresponding DOM node must be deleted at which time the // renderer itself will have been already deleted and with it the map and all // the counters via RenderCounter::destroyCounters() or a style change // involving removal of all counter directives must occur in which case again // RenderCounter::destroyCounters() must be called. Once I have this information, I can submit a patch that answers all your concerns.

(In reply to comment #17) > B. Should I change the comment to say something like this: > // The counterMap is not deleted here even if empty because we expect it to > // be reused soon. In order for a renderer to loose all its counters > // permanently, the corresponding DOM node must be deleted at which time the > // renderer itself will have been already deleted and with it the map and all > // the counters via RenderCounter::destroyCounters() or a style change > // involving removal of all counter directives must occur in which case again > // RenderCounter::destroyCounters() must be called. B. Something like that is OK. The comment is much easier to understand, although still a little subtle. Typo: "loose".

Created attachment 46708 [details] Proposed patch; Addressed Darin's concerns.

Comment on attachment 46708 [details] Proposed patch; Addressed Darin's concerns. Clearing flags on attachment: 46708 Committed r53355: <http://trac.webkit.org/changeset/53355>

All reviewed patches have been landed. Closing bug.