Bug 123016

Summary:

[CSS Regions] Possible performance regression after r157567

Product:

WebKit

Reporter:

Andrei Bucur <abucur>

Component:

WebCore Misc.

Assignee:

Andrei Bucur <abucur>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cmarcelo, commit-queue, esprehn+autocc, glenn, kangil.han, kling, kondapallykalyan, mibalan, mihnea, WebkitBugTracker

Priority:

Keywords:

AdobeTracked

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

57312

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none
Patch for landing + Win build fix	none

Andrei Bucur

Reported 2013-10-18 04:06:41 PDT

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

Attachments
Patch (10.67 KB, patch) 2013-10-18 06:01 PDT, Andrei Bucur	no flags	Details Formatted Diff Diff
Patch (25.50 KB, patch) 2013-10-21 10:09 PDT, Andrei Bucur	no flags	Details Formatted Diff Diff
Patch for landing (25.55 KB, patch) 2013-10-22 04:09 PDT, Andrei Bucur	no flags	Details Formatted Diff Diff
Patch for landing + Win build fix (26.26 KB, patch) 2013-10-22 06:58 PDT, Andrei Bucur	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Andrei Bucur

Comment 1 2013-10-18 06:01:36 PDT

Created attachment 214562 [details] Patch

Andrei Bucur

Comment 2 2013-10-18 07:48:24 PDT

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 3 2013-10-18 13:35:11 PDT

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

Comment 4 2013-10-21 10:09:49 PDT

Created attachment 214747 [details] Patch

Andreas Kling

Comment 5 2013-10-22 02:11:40 PDT

Comment on attachment 214747 [details] Patch Looks cool! Thanks for taking the time to tweak these things :)

WebKit Commit Bot

Comment 6 2013-10-22 02:30:57 PDT

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

Comment 7 2013-10-22 04:09:44 PDT

Created attachment 214835 [details] Patch for landing

Build Bot

Comment 8 2013-10-22 04:47:03 PDT

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

Comment 9 2013-10-22 06:58:53 PDT

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

WebKit Commit Bot

Comment 10 2013-10-22 07:48:56 PDT

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

Comment 11 2013-10-22 07:48:59 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.