[Webkit-unassigned] [Bug 11031] Another crazy counters bug
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Nov 20 10:23:23 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=11031
--- Comment #28 from Carol Szabo <carol.szabo at nokia.com> 2009-11-20 10:23:22 PST ---
(In reply to comment #27)
> (From update of attachment 43487 [details])
> Patch looks good. Quite a few stylistic issues.
>
> I don't know that all of this needs to go in one patch. I'd prefer to see one
> fix at a time even though it would take a bit longer.
> In WebKit coding style this does not have braces.
>
> > - bool isReset() const { return m_isReset; }
> > + bool isReset() const { return m_isReset || !m_parent; }
> > + bool isTypeReset() const { return m_isReset; }
>
> I don't think these names are clear enough.
> > + // identifier must match the identifier of this counter or
> > + // the renderers affected by this removal shall not be invalidated.
> > void removeChild(CounterNode*, const AtomicString& identifier);
>
> This comment seems kind of sideways.
> We sometimes use "identifier" and other times "counterName". It would be good
> to make the terminology as consistent as possible.
>
Eric Seidel and the commit bot wants one landed patch per bug hence I created
bug 31723 (https://bugs.webkit.org/show_bug.cgi?id=31723) which highlights the
problems in the findPlaceForCounter algorithm and I created a small fix just
around that.
I hate pedantic fixes that require bug filings and going through a whole review
process just to add comments to code and change variable names which add no
benefit to the user, and are beneficial to a developer only in the context of
looking at a real user issue, so I put the comments and related fixes in this
patch together with this algorithm fix hopefully the reviewer (likely Darin)
will not mind that.
> > +// Finds the insertion point for the counter described by counterOwner, isReset and
> > +// counterName in the CounterNode tree for counterName and sets parent and
> > +// previousSibling accordingly.
> > +// The function returns true if the counter whose insertion point is searched is NOT
> > +// the root of the tree.
> > +// The root of the tree is a counter reference that is not in the scope of any other
> > +// counter with the same identifier.
>
> Typically we don't put line breaks after sentences in a comment like this.
I found myself struggling to understand the design of CounterNode trees so for
the benefit of whoever looks at this code after me I took the opportunity to
clarify a few things here that are critical for the implementation of this
function beside the boilerplate description of what the function does and what
the return values are. Since I had a lot of explaining to do, I used line
breaks to separate main ideas, try to make the comment more readable while
keeping it pretty condensed. I am sorry if I failed, I tried a new (hopefully
better) version in the patch to 31723 (see above).
> > CounterMap::const_iterator end = map->end();
> > for (CounterMap::const_iterator it = map->begin(); it != end; ++it) {
> > - CounterNode* node = it->second;
> > AtomicString identifier(it->first.get());
> > - destroyCounterNodeChildren(identifier, node);
> > - if (CounterNode* parent = node->parent())
> > - parent->removeChild(node, identifier);
> > - delete node;
> > + destroyCounterNodeWithoutMapRemoval(identifier, it->second);
> > }
>
> Braces go now that the body has one line.
The body is actually 2 lines: AtomicString identifier(it->first.get()); stays.
> > +void RenderCounter::updateCounters(RenderObject* renderer)
> > +{
> > + updateCounter(renderer);
> > + for (RenderObject* child = renderer->nextInPreOrder(renderer); child; child = child->nextInPreOrder(renderer))
> > + updateCounter(child);
> > +}
>
> The variable child is actually any descendant, so child is not the optimal
> name. And this can be written like this:
>
> for (RenderObject* descendant = renderer; descendant; descendant =
> descendant->nextInPreOrder(renderer))
> updateCounter(descendant);
>
> No separate function call outside the loop needed.
I have refactored the code per your comment. You are right about
child/descendant, as far as including the initial updateCounter in the loop, I
was trying to save an unnecessary comparison, since this function is called for
every DOM node insertion I was trying to make it as quick as possible if the
node being inserted and its descendants have no counter directives. (See the
next item bellow about updateCounters being called unnecessarily).
> > + RenderCounter::updateCounters(newChild);
>
> Is this extra work that is bad for performance?
As I explained in my response to Shinchiro Hamaji, this work is usually not
that much and is critical for being able to respond to DOM node changes.
In the current implementation updateCounters, for most DOM nodes, boils down
to: 2 function calls with one pointer arg, plus checking the style of the node
for counter directives and the node for the existence of children. I believe
this is the bare minimum needed to detect changes to the counter hierarchy that
result from addition of DOM nodes.
The reason why Shinchiro's "check before the call" solution (which only saves
the time to call 2 non virtual functions - pretty much one CPU cycle on
processors with long enough instruction pipelines) does not work is that
sometimes a whole Renderer subtree is inserted at once in the main tree and
sometimes the root node of this renderer tree does not have any counter
directive such as in this case:
<span style=":before reset-counter c 4;"/>
The Renderer for span and for its virtual before child are inserted together in
the main tree, but the renderer for span has no counter directives.
I found this case in one of the css2.1 tests.
> > - updateFillImages(oldStyle ? oldStyle->backgroundLayers() : 0, m_style ? m_style->backgroundLayers() : 0);
> > - updateFillImages(oldStyle ? oldStyle->maskLayers() : 0, m_style ? m_style->maskLayers() : 0);
> > -
> > - updateImage(oldStyle ? oldStyle->borderImage().image() : 0, m_style ? m_style->borderImage().image() : 0);
> > - updateImage(oldStyle ? oldStyle->maskBoxImage().image() : 0, m_style ? m_style->maskBoxImage().image() : 0);
> > + if (oldStyle) {
> > + if (m_style) {
> > + if (const CounterDirectiveMap* pCounterDirectiveMap = m_style->counterDirectives()) {
> > + if (const CounterDirectiveMap* pOldCounterDirectiveMap = oldStyle->counterDirectives()) {
> > + CounterDirectiveMap::const_iterator end = pCounterDirectiveMap->end();
> > + for (CounterDirectiveMap::const_iterator it = pCounterDirectiveMap->begin(); it != end; ++it) {
> > + if (pOldCounterDirectiveMap->contains(it->first)) {
> > + if (pOldCounterDirectiveMap->get(it->first) != it->second)
> > + RenderCounter::destroyCounterNode(this, AtomicString(it->first.get()));
> > + } else {
> > + RenderCounter::destroyCounterNode(this, AtomicString(it->first.get()));
> > + RenderCounter::createCounterIfNeeded(this, AtomicString(it->first.get()));
> > + }
> > + }
> > + end = pOldCounterDirectiveMap->end();
> > + for (CounterDirectiveMap::const_iterator it = pOldCounterDirectiveMap->begin(); it != end; ++it)
> > + if (!pCounterDirectiveMap->contains(it->first))
> > + RenderCounter::destroyCounterNode(this, AtomicString(it->first.get()));
> > + } else {
> > + RenderCounter::destroyCounterNodes(this);
> > + RenderCounter::updateCounters(this);
> > + }
> > + } else if (m_hasCounterNodeMap)
> > + RenderCounter::destroyCounterNodes(this);
> >
> > - // We need to ensure that view->maximalOutlineSize() is valid for any repaints that happen
> > - // during styleDidChange (it's used by clippedOverflowRectForRepaint()).
> > - if (m_style->outlineWidth() > 0 && m_style->outlineSize() > maximalOutlineSize(PaintPhaseOutline))
> > - toRenderView(document()->renderer())->setMaximalOutlineSize(m_style->outlineSize());
> > + updateFillImages(oldStyle->backgroundLayers(),
> > + m_style->backgroundLayers());
> > + updateFillImages(oldStyle->maskLayers(), m_style->maskLayers());
> > +
> > + updateImage(oldStyle->borderImage().image(),
> > + m_style->borderImage().image());
> > + updateImage(oldStyle->maskBoxImage().image(),
> > + m_style->maskBoxImage().image());
> > + // We need to ensure that view->maximalOutlineSize() is valid for
> > + // any repaints that happen during styleDidChange (it's used by
> > + // clippedOverflowRectForRepaint()).
> > + if (m_style->outlineWidth() > 0 && m_style->outlineSize() >
> > + maximalOutlineSize(PaintPhaseOutline)) {
> > + toRenderView(document()->renderer())->setMaximalOutlineSize(
> > + m_style->outlineSize());
> > + }
> > + } else {
> > + if (m_hasCounterNodeMap)
> > + RenderCounter::destroyCounterNodes(this);
> > + updateFillImages(oldStyle->backgroundLayers(), 0);
> > + updateFillImages(oldStyle->maskLayers(), 0);
> > +
> > + updateImage(oldStyle->borderImage().image(), 0);
> > + updateImage(oldStyle->maskBoxImage().image(), 0);
> > + }
> > + } else if (m_style) {
> > + RenderCounter::updateCounter(this);
> > + updateFillImages(0, m_style->backgroundLayers());
> > + updateFillImages(0, m_style->maskLayers());
> > +
> > + updateImage(0, m_style->borderImage().image());
> > + updateImage(0, m_style->maskBoxImage().image());
> > + // We need to ensure that view->maximalOutlineSize() is valid for any repaints that happen
> > + // during styleDidChange (it's used by clippedOverflowRectForRepaint()).
> > + if (m_style->outlineWidth() > 0 && m_style->outlineSize() > maximalOutlineSize(PaintPhaseOutline))
> > + toRenderView(document()->renderer())->setMaximalOutlineSize(m_style->outlineSize());
> > + }
>
> This new code is long and hard to read and understand. What's the benefit of
> putting all the cases in different branches like this? Is there a way we can
> factor this so it's not so gigantic? Replacing 4 lines of code with 50 needs a
> really good justification.
The code above appears to replace 4 lines, but in reality it adds totally new
functionality while optimizing those 4 lines to run faster.
The design goal behind the code above which I admit has some readability issues
(mainly due to broken lines, which I fixed) is to make the code as fast as
possible since it is run for every style change.
I read an article in a respected Software Development journal where a guy
published a breadth first graph travesal algorithm optimized for the Cell
processor. The algorithm had 5000 lines versus the usual about 50. But was
running an order of magnitude faster on the same hardware as the vanilla
algorithm.
My goal in the design of the code above was to minimize unnecessary work.
In the original code the 4 or so functions were called whether or not it made
sense (i.e. whether or not there was a non-null oldStyle or newStyle).
I know from testing that sometimes oldStyle can be null and probably newStyle
may be null too.
So I separated the code in these 4 cases and I minimized work in each of them.
For counters the problem is complicated by the fact that for each style there
may or may not be counter directives associated with the style, and the
counters may or may not be the same for each directive.
Of course there is an easy way out: delete all old counters if any in all cases
and create the new ones if any, but this is way less than optimal as creating a
counter involves inspecting many renderers.
I took your comment to heart though and I shall try to create some helper
functions to increase readability and I shall definitely stop breaking those
lines. I personally hate breaking the lines but I read somewhere in the style
guidelines that long lines are bad and I was trying to comply with that.
Darin,
Please review my patch for 31723 so that I can come back and submit a smaller
better patch for this.
Thanks.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list