Bug 123016 – [CSS Regions] Possible performance regression after r157567

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Andrei Bucur]

r157567 may have regressed Parser/html5-full-render by ~1.1% and Parser/html-parser by ~2%:

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%2C%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml-parser%3ATime%22%5D%5D

[Andrei Bucur]

Created attachment 214562 [details]
Patch

[Andrei Bucur]

Performance results on Parser/html-parser:

With the patch:
1438.45	± 0.35%	
1439.70	± 0.18%
1434.50	± 0.27%
1432.70	± 0.22%	0.40% Better
1432.05	± 0.27%	0.44% Better

Without the patch:
1449.10	± 0.41%	0.74% Worse
1459.05	± 0.45%	1.43% Worse
1460.35	± 0.42%	1.52% Worse
1450.25	± 0.33%	0.82% Worse
1447.80	± 0.28%	0.65% Worse

[Andreas Kling]

Comment on attachment 214562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214562&action=review

Thanks for looking into this! Some ponderings:

> Source/WebCore/rendering/RenderBlockFlow.cpp:-280
> -    if (logicalWidthChangedInRegions(flowThread))
> +    if (logicalWidthChangedInRegions(flowThread) || namedFlowFragmentNeedsUpdate())
>          relayoutChildren = true;
>      if (updateShapesBeforeBlockLayout())
>          relayoutChildren = true;
> -    if (namedFlowFragmentNeedsUpdate())
> -        relayoutChildren = true;

Would it make sense to do the updateShapesBeforeBlockLayout() check first, and then skip the other two checks if relayoutChildren already got set to true?

> Source/WebCore/rendering/RenderObject.cpp:-2567
> -bool RenderObject::isRenderNamedFlowFragmentContainer() const
> -{
> -    if (!isRenderBlockFlow())
> -        return false;
> -
> -    return toRenderBlockFlow(this)->renderNamedFlowFragment();
> -}

Why not just move this function to RenderElement instead?
Making it virtual seems strictly more expensive, since we never actually call it through a RenderBlockFlow pointer.

[Andrei Bucur]

Created attachment 214747 [details]
Patch

[Andreas Kling]

Comment on attachment 214747 [details]
Patch

Looks cool! Thanks for taking the time to tweak these things :)

[WebKit Commit Bot]

Comment on attachment 214747 [details]
Patch

Rejecting attachment 214747 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 214747, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ILED at 45.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderNamedFlowFragment.h.rej
patching file Source/WebCore/rendering/RenderObject.cpp
patching file Source/WebCore/rendering/RenderObject.h
patching file Source/WebCore/rendering/RenderTreeAsText.cpp
patching file Source/WebCore/style/StyleResolveTree.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Andreas Kling']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/8868221

[Andrei Bucur]

Created attachment 214835 [details]
Patch for landing

[Build Bot]

Comment on attachment 214835 [details]
Patch for landing

Attachment 214835 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/8818256

[Andrei Bucur]

Created attachment 214853 [details]
Patch for landing + Win build fix

[WebKit Commit Bot]

Comment on attachment 214853 [details]
Patch for landing + Win build fix

Clearing flags on attachment: 214853

Committed r157793: <http://trac.webkit.org/changeset/157793>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.