| Summary: | Move RenderObject::markContainingBlocksForLayout() to RenderObject.cpp | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||
| Component: | Layout and Rendering | Assignee: | Kent Tamura <tkent> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Minor | CC: | darin, hyatt, jamesr, simon.fraser, webkit.review.bot | ||||||||||
| Priority: | P3 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Kent Tamura
2011-09-13 01:15:21 PDT
Created attachment 107150 [details]
Patch
Attachment 107150 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderObject.cpp:597: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 107150 [details] Patch Attachment 107150 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9654054 Comment on attachment 107150 [details] Patch Attachment 107150 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9660046 Comment on attachment 107150 [details] Patch Attachment 107150 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9651102 Comment on attachment 107150 [details] Patch Attachment 107150 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9655050 Oops, I didn't check Release build. Created attachment 107154 [details]
Patch 2
Fixed Release build
We'll need some performance measurements before accepting this patch. Comment on attachment 107154 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=107154&action=review > Source/WebCore/ChangeLog:3 > + Move RenderObject::markContainingBlocksForLayout() to RenderObject.cpp. You said you wanted to move this so you could modify it without recompiling. That’s OK, but what about the reason it was originally marked inline? Does changing this make certain operations slower or faster? WebKit code bigger or smaller? > Source/WebCore/rendering/RenderObject.cpp:587 > +inline bool objectIsRelayoutBoundary(const RenderObject *obj) A function that’s internal to a cpp file like this should be marked “static” so that it gets internal linkage. That’s needed whether it’s inline or not. If moving this, I think we need to fix the basic style mistakes in it. The * needs to be moved next to RenderObject. The argument name should be object, not "obj". > Source/WebCore/rendering/RenderObject.cpp:603 > + RenderObject* o = container(); This should be object, not "o" or perhaps an even better name is available. Created attachment 107304 [details]
Performance measurement result
Load 153 sites in Chromium's PLT 10 times without the patch and 10 times with the patch.
(In reply to comment #11) > Created an attachment (id=107304) [details] > Performance measurement result > > Load 153 sites in Chromium's PLT 10 times without the patch and 10 times with the patch. The following list shows total loading times by r95072 and by r95072 + the patch. The total loading time doesn't contain the first loading. Result: * The patch didn't show significant difference. However, loading times were slightly better for 100 sites. * The patch will reduce the size of Chromium by 74KB on i386 Mac. bugzilla.mozilla.org; r95072=251msec; Patched=250msec; Time percentage=99.6% espn.go.com; r95072=400msec; Patched=399msec; Time percentage=99.75% home.netscape.com; r95072=324msec; Patched=318msec; Time percentage=98.14% hotwired.lycos.com; r95072=257msec; Patched=271msec; Time percentage=105.44% lxr.mozilla.org; r95072=459msec; Patched=465msec; Time percentage=101.3% my.netscape.com; r95072=293msec; Patched=288msec; Time percentage=98.29% news.cnet.com; r95072=284msec; Patched=277msec; Time percentage=97.53% slashdot.org; r95072=295msec; Patched=289msec; Time percentage=97.96% vanilla-page; r95072=89msec; Patched=89msec; Time percentage=100% web.icq.com; r95072=434msec; Patched=406msec; Time percentage=93.54% www.altavista.com; r95072=202msec; Patched=198msec; Time percentage=98.01% www.amazon.com; r95072=255msec; Patched=255msec; Time percentage=100% www.aol.com; r95072=400msec; Patched=386msec; Time percentage=96.5% www.apple.com; r95072=135msec; Patched=136msec; Time percentage=100.74% www.cnn.com; r95072=559msec; Patched=531msec; Time percentage=94.99% www.compuserve.com; r95072=267msec; Patched=264msec; Time percentage=98.87% www.digitalcity.com; r95072=254msec; Patched=248msec; Time percentage=97.63% www.ebay.com; r95072=238msec; Patched=227msec; Time percentage=95.37% www.excite.com; r95072=349msec; Patched=334msec; Time percentage=95.7% www.expedia.com; r95072=330msec; Patched=337msec; Time percentage=102.12% www.google.com; r95072=81msec; Patched=79msec; Time percentage=97.53% www.iplanet.com; r95072=157msec; Patched=161msec; Time percentage=102.54% www.mapquest.com; r95072=145msec; Patched=149msec; Time percentage=102.75% www.microsoft.com; r95072=177msec; Patched=183msec; Time percentage=103.38% www.moviefone.com; r95072=250msec; Patched=239msec; Time percentage=95.6% www.msn.com; r95072=392msec; Patched=394msec; Time percentage=100.51% www.msnbc.com; r95072=181msec; Patched=179msec; Time percentage=98.89% www.nytimes.com; r95072=342msec; Patched=354msec; Time percentage=103.5% www.nytimes.com_Table; r95072=583msec; Patched=584msec; Time percentage=100.17% www.quicken.com; r95072=216msec; Patched=213msec; Time percentage=98.61% www.spinner.com; r95072=378msec; Patched=379msec; Time percentage=100.26% www.sun.com; r95072=137msec; Patched=139msec; Time percentage=101.45% www.time.com; r95072=403msec; Patched=418msec; Time percentage=103.72% www.tomshardware.com; r95072=527msec; Patched=530msec; Time percentage=100.56% www.travelocity.com; r95072=393msec; Patched=294msec; Time percentage=74.8% www.voodooextreme.com; r95072=1085msec; Patched=1124msec; Time percentage=103.59% www.w3.org_DOML2Core; r95072=1098msec; Patched=1089msec; Time percentage=99.18% www.wired.com; r95072=399msec; Patched=396msec; Time percentage=99.24% www.yahoo.com; r95072=170msec; Patched=169msec; Time percentage=99.41% www.zdnet.com; r95072=523msec; Patched=519msec; Time percentage=99.23% www.zdnet.com_Gamespot.com; r95072=455msec; Patched=450msec; Time percentage=98.9% 126.com; r95072=530msec; Patched=509msec; Time percentage=96.03% 2ch.net; r95072=185msec; Patched=192msec; Time percentage=103.78% 6park.com; r95072=4055msec; Patched=3935msec; Time percentage=97.04% affili.net; r95072=371msec; Patched=359msec; Time percentage=96.76% allegro.pl; r95072=1030msec; Patched=990msec; Time percentage=96.11% apeha.ru; r95072=670msec; Patched=651msec; Time percentage=97.16% baidu.com; r95072=107msec; Patched=104msec; Time percentage=97.19% bbs.wefong.com; r95072=2078msec; Patched=2069msec; Time percentage=99.56% blog.skyrock.com; r95072=1070msec; Patched=1003msec; Time percentage=93.73% cmfu.com; r95072=3056msec; Patched=3040msec; Time percentage=99.47% cn.yahoo.com; r95072=1701msec; Patched=1697msec; Time percentage=99.76% contra.gr; r95072=641msec; Patched=644msec; Time percentage=100.46% dtiblog.com; r95072=2629msec; Patched=2638msec; Time percentage=100.34% el.wikipedia.org; r95072=939msec; Patched=977msec; Time percentage=104.04% elmundo.es; r95072=1211msec; Patched=1361msec; Time percentage=112.38% ettoday.com; r95072=4684msec; Patched=4878msec; Time percentage=104.14% exblog.jp; r95072=2104msec; Patched=2001msec; Time percentage=95.1% excite.co.jp; r95072=624msec; Patched=650msec; Time percentage=104.16% fc2.com; r95072=2436msec; Patched=2486msec; Time percentage=102.05% fora.pl; r95072=987msec; Patched=865msec; Time percentage=87.63% free.fr; r95072=803msec; Patched=799msec; Time percentage=99.5% golem.de; r95072=510msec; Patched=507msec; Time percentage=99.41% goo.ne.jp; r95072=3301msec; Patched=3280msec; Time percentage=99.36% haberturk.com; r95072=1676msec; Patched=1665msec; Time percentage=99.34% hatena.ne.jp; r95072=878msec; Patched=1086msec; Time percentage=123.69% home.altervista.org; r95072=665msec; Patched=462msec; Time percentage=69.47% hurriyet.com.tr; r95072=1303msec; Patched=1321msec; Time percentage=101.38% jugem.jp; r95072=2570msec; Patched=2389msec; Time percentage=92.95% kakaku.com; r95072=1619msec; Patched=1577msec; Time percentage=97.4% mixi.jp; r95072=456msec; Patched=439msec; Time percentage=96.27% naftemporiki.gr; r95072=2240msec; Patched=2351msec; Time percentage=104.95% narod.yandex.ru; r95072=320msec; Patched=344msec; Time percentage=107.5% news.163.com; r95072=6477msec; Patched=6522msec; Time percentage=100.69% partyflock.nl; r95072=913msec; Patched=1059msec; Time percentage=115.99% pchome.com.tw; r95072=1024msec; Patched=1401msec; Time percentage=136.81% phoenixtv.com; r95072=3407msec; Patched=3638msec; Time percentage=106.78% photofile.ru; r95072=1556msec; Patched=1479msec; Time percentage=95.05% pl.wikipedia.org; r95072=1376msec; Patched=1221msec; Time percentage=88.73% ricardo.ch; r95072=445msec; Patched=568msec; Time percentage=127.64% ru.wikipedia.org; r95072=1062msec; Patched=1022msec; Time percentage=96.23% ruten.com.tw; r95072=1435msec; Patched=1311msec; Time percentage=91.35% sport24.gr; r95072=1401msec; Patched=1395msec; Time percentage=99.57% terra.es; r95072=1293msec; Patched=1284msec; Time percentage=99.3% udn.com; r95072=3533msec; Patched=3554msec; Time percentage=100.59% uwants.com; r95072=2370msec; Patched=2433msec; Time percentage=102.65% voila.fr; r95072=989msec; Patched=986msec; Time percentage=99.69% www.alice.it; r95072=2769msec; Patched=2930msec; Time percentage=105.81% www.amazon.co.jp; r95072=1000msec; Patched=1005msec; Time percentage=100.49% www.auction.co.kr; r95072=4343msec; Patched=5969msec; Time percentage=137.43% www.chinaren.com; r95072=4068msec; Patched=4458msec; Time percentage=109.58% www.chosun.com; r95072=4767msec; Patched=4825msec; Time percentage=101.21% www.danawa.com; r95072=4589msec; Patched=4578msec; Time percentage=99.76% www.daum.net; r95072=2344msec; Patched=2281msec; Time percentage=97.31% www.dcinside.com; r95072=2619msec; Patched=2596msec; Time percentage=99.12% www.eastmoney.com; r95072=1848msec; Patched=1781msec; Time percentage=96.37% zol.com.cn; r95072=4898msec; Patched=4633msec; Time percentage=94.58% arabicnews.google.com; r95072=3480msec; Patched=3467msec; Time percentage=99.62% bn.wikipedia.org; r95072=1213msec; Patched=1220msec; Time percentage=100.57% exteen.com; r95072=1151msec; Patched=1150msec; Time percentage=99.91% farsnews.com; r95072=2283msec; Patched=2278msec; Time percentage=99.78% hindi.webdunia.com; r95072=2566msec; Patched=2521msec; Time percentage=98.24% in.telugu.yahoo.com; r95072=640msec; Patched=684msec; Time percentage=106.87% isna.ir; r95072=2678msec; Patched=2655msec; Time percentage=99.14% kapook.com; r95072=2778msec; Patched=2873msec; Time percentage=103.41% kooora.com; r95072=2854msec; Patched=2840msec; Time percentage=99.5% manager.co.th; r95072=5970msec; Patched=5963msec; Time percentage=99.88% masrawy.com; r95072=1766msec; Patched=1765msec; Time percentage=99.94% ml.wikipedia.org; r95072=1454msec; Patched=1448msec; Time percentage=99.58% msn.co.il; r95072=1631msec; Patched=1570msec; Time percentage=96.25% news.bbc.co.uk; r95072=1128msec; Patched=1118msec; Time percentage=99.11% news.google.com; r95072=2897msec; Patched=2883msec; Time percentage=99.51% sh3bwah.com; r95072=3321msec; Patched=3310msec; Time percentage=99.66% sgkalesh.blogspot.com; r95072=1814msec; Patched=1701msec; Time percentage=93.77% tapuz.co.il; r95072=798msec; Patched=794msec; Time percentage=99.49% thaimisc.com; r95072=1524msec; Patched=1502msec; Time percentage=98.55% vietnamnet.vn; r95072=1306msec; Patched=1295msec; Time percentage=99.15% vnexpress.net; r95072=780msec; Patched=836msec; Time percentage=107.17% walla.co.il; r95072=589msec; Patched=591msec; Time percentage=100.33% www.aljayyash.net; r95072=2280msec; Patched=2238msec; Time percentage=98.15% www.bbc.co.uk; r95072=900msec; Patched=969msec; Time percentage=107.66% www.google.com.sa; r95072=131msec; Patched=129msec; Time percentage=98.47% www.islamweb.net; r95072=2260msec; Patched=2346msec; Time percentage=103.8% www.mthai.com; r95072=3963msec; Patched=3944msec; Time percentage=99.52% www.startimes2.com; r95072=4708msec; Patched=4680msec; Time percentage=99.4% www.jagran.com; r95072=2726msec; Patched=2724msec; Time percentage=99.92% ynet.co.il; r95072=1721msec; Patched=1716msec; Time percentage=99.7% colorfade; r95072=5148msec; Patched=5135msec; Time percentage=99.74% diagball; r95072=6168msec; Patched=6140msec; Time percentage=99.54% fadespacing; r95072=8290msec; Patched=8271msec; Time percentage=99.77% imageslide; r95072=1403msec; Patched=1396msec; Time percentage=99.5% layers1; r95072=1282msec; Patched=1312msec; Time percentage=102.34% layers2; r95072=220msec; Patched=223msec; Time percentage=101.36% layers4; r95072=218msec; Patched=221msec; Time percentage=101.37% layers5; r95072=886msec; Patched=866msec; Time percentage=97.74% layers6; r95072=314msec; Patched=237msec; Time percentage=75.47% meter; r95072=4071msec; Patched=4053msec; Time percentage=99.55% movingtext; r95072=2864msec; Patched=2864msec; Time percentage=100% mozilla; r95072=10360msec; Patched=10320msec; Time percentage=99.61% replaceimages; r95072=1220msec; Patched=1211msec; Time percentage=99.26% scrolling; r95072=1691msec; Patched=1704msec; Time percentage=100.76% slidein; r95072=9641msec; Patched=9621msec; Time percentage=99.79% slidingballs; r95072=1030msec; Patched=976msec; Time percentage=94.75% zoom; r95072=2035msec; Patched=2024msec; Time percentage=99.45% blog.chromium.org; r95072=2439msec; Patched=3027msec; Time percentage=124.1% dev.chromium.org; r95072=2271msec; Patched=2256msec; Time percentage=99.33% googleblog.blogspot.com1; r95072=767msec; Patched=761msec; Time percentage=99.21% googleblog.blogspot.com2; r95072=1482msec; Patched=1468msec; Time percentage=99.05% test.blogspot.com; r95072=2528msec; Patched=2522msec; Time percentage=99.76% www.igoogle.com; r95072=1904msec; Patched=1840msec; Time percentage=96.63% www.techcrunch.com; r95072=1915msec; Patched=1941msec; Time percentage=101.35% www.webkit.org; r95072=1769msec; Patched=1763msec; Time percentage=99.66% www.yahoo.com; r95072=1290msec; Patched=1310msec; Time percentage=101.55% Created attachment 107308 [details]
Patch 3
Follows Darin's comments
Comment on attachment 107154 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=107154&action=review Thank you for the comments. >> Source/WebCore/rendering/RenderObject.cpp:587 >> +inline bool objectIsRelayoutBoundary(const RenderObject *obj) > > A function that’s internal to a cpp file like this should be marked “static” so that it gets internal linkage. That’s needed whether it’s inline or not. > > If moving this, I think we need to fix the basic style mistakes in it. The * needs to be moved next to RenderObject. The argument name should be object, not "obj". Added 'static'. Fixed to 'RenderObject* object' >> Source/WebCore/rendering/RenderObject.cpp:603 >> + RenderObject* o = container(); > > This should be object, not "o" or perhaps an even better name is available. Renamed it to 'object.' Comment on attachment 107308 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=107308&action=review > Source/WebCore/rendering/RenderObject.cpp:596 > + || (object->hasOverflowClip() && !object->style()->width().isIntrinsicOrAuto() && !object->style()->height().isIntrinsicOrAuto() && !object->style()->height().isPercent() && !object->isTableCell()); Can we split this into two lines? This line seems too long. Simon, Darin, do you have further comments? (In reply to comment #16) > Simon, Darin, do you have further comments? I am slightly concerned that total time spent on a page load test is not a thorough enough way to test the performance impact. Otherwise, I am happy with the change and I don’t want to stand in the way of it getting checked in. Comment on attachment 107308 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=107308&action=review >> Source/WebCore/rendering/RenderObject.cpp:596 >> + || (object->hasOverflowClip() && !object->style()->width().isIntrinsicOrAuto() && !object->style()->height().isIntrinsicOrAuto() && !object->style()->height().isPercent() && !object->isTableCell()); > > Can we split this into two lines? This line seems too long. Since this is now a separate function it could be written as a set of if statements instead of a single expression, and doing that might make it easier to read and eliminate the need for this long line, and even provide room for more “why” comments. Not needed n this patch, though. (In reply to comment #17) > I am slightly concerned that total time spent on a page load test is not a thorough enough way to test the performance impact. The tests 'colorfade' through 'zoom' are animation tests by updating DOM by JavaScript. I think their results showed this change had no performance impact. Comment on attachment 107308 [details]
Patch 3
I'll land this patch as is. Feel free to post another patch for objectIsRelayoutBoundary() :-)
Comment on attachment 107308 [details] Patch 3 Clearing flags on attachment: 107308 Committed r95598: <http://trac.webkit.org/changeset/95598> All reviewed patches have been landed. Closing bug. |