[Webkit-unassigned] [Bug 129065] Render tree construction is O(N^2) in number of siblings
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 25 09:35:51 PDT 2014
https://bugs.webkit.org/show_bug.cgi?id=129065
Darin Adler <darin at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #227760|review? |review+
Flag| |
--- Comment #3 from Darin Adler <darin at apple.com> 2014-03-25 09:36:10 PST ---
(From update of attachment 227760)
View in context: https://bugs.webkit.org/attachment.cgi?id=227760&action=review
The approach here looks good. The details are a little confusing.
> Source/WebCore/style/StyleResolveTree.cpp:7
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2012, 2013, 2014 Apple Inc. All rights reserved.
We learned recently that year ranges are OK. So this should say:
2004-2010, 2012-2014
> Source/WebCore/style/StyleResolveTree.cpp:75
> + RenderObject* before;
The name “before” here is not so great. We name the other member “parent”, not “above” or “containing”, despite the fact that it’s not really the parent of anything. I think this might be clearer as “nextSibling”.
> Source/WebCore/style/StyleResolveTree.cpp:76
> + bool isValid;
I don’t think that from this name it’s clear that isValid is specifically about the before field. I think it should probably be named nextSiblingPointerIsValid.
I find that the rules for when a RenderTreePosition is valid and when it might not be are confusing.
> Source/WebCore/style/StyleResolveTree.cpp:78
> + unsigned assertCount;
I would call this assertionLimitCounter.
> Source/WebCore/style/StyleResolveTree.cpp:237
> +static void computeRenderTreePositionIfNeeded(RenderTreePosition& renderTreePosition, const Node& node, ContainerNode& renderingParentNode)
I don’t think the IfNeeded suffix in these two function names helps. I would just take it out.
> Source/WebCore/style/StyleResolveTree.cpp:249
> +static void invalidateRenderTreePositionIfNeeded(RenderTreePosition& renderTreePosition, const Node& node)
I don’t think the IfNeeded suffix in these two function names helps. I would just take it out.
> Source/WebCore/style/StyleResolveTree.cpp:280
> + ASSERT(renderTreePosition.isValid);
Making the position a little more of a class would be good. Then the assertion that the pointer is valid could be a part of the getter rather than something we assert here, but use elsewhere.
> Source/WebCore/style/StyleResolveTree.cpp:285
> + insertionPosition.before = parentFlowRenderer->nextRendererForElement(element);
It’s strange and subtle that we rely on the fact that isValid is already set here.
> LayoutTests/fast/css/sibling-renderer-On2.html:11
> +shouldBe("timeRenderTreeCreation(5000) < 20 * timeRenderTreeCreation(500)", "true");
I’m surprised you made a custom test here. I would have suggested instead doing a LayoutTests/perf test; those are designed to check that things are linear.
But perhaps the number "20" here is related to the the number "20" in the assertion limit count for debug builds?
--
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