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: | P2 | Keywords: | AdobeTracked | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 57312 | ||||||||||||
Attachments: |
|
Description
Andrei Bucur
2013-10-18 04:06:41 PDT
Created attachment 214562 [details]
Patch
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 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. Created attachment 214747 [details]
Patch
Comment on attachment 214747 [details]
Patch
Looks cool! Thanks for taking the time to tweak these things :)
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 Created attachment 214835 [details]
Patch for landing
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 Created attachment 214853 [details]
Patch for landing + Win build fix
Comment on attachment 214853 [details] Patch for landing + Win build fix Clearing flags on attachment: 214853 Committed r157793: <http://trac.webkit.org/changeset/157793> All reviewed patches have been landed. Closing bug. |