Bug 67989 - Move RenderObject::markContainingBlocksForLayout() to RenderObject.cpp
Summary: Move RenderObject::markContainingBlocksForLayout() to RenderObject.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-13 01:15 PDT by Kent Tamura
Modified: 2011-09-20 22:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.97 KB, patch)
2011-09-13 01:18 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (6.95 KB, patch)
2011-09-13 02:25 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Performance measurement result (13.60 KB, text/plain)
2011-09-14 01:14 PDT, Kent Tamura
no flags Details
Patch 3 (7.10 KB, patch)
2011-09-14 01:55 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-09-13 01:15:21 PDT
I'd like to move RenderObject::markContainingBlocksForLayout() from RenderObject.h to RenderObject.cpp.
It's too large in a header file, and I hesitate to modify it because updating RenderObject.h causes recompilation of many files.
Comment 1 Kent Tamura 2011-09-13 01:18:24 PDT
Created attachment 107150 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-13 01:21:11 PDT
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 3 Early Warning System Bot 2011-09-13 01:31:06 PDT
Comment on attachment 107150 [details]
Patch

Attachment 107150 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9654054
Comment 4 Gyuyoung Kim 2011-09-13 01:33:14 PDT
Comment on attachment 107150 [details]
Patch

Attachment 107150 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9660046
Comment 5 WebKit Review Bot 2011-09-13 01:35:57 PDT
Comment on attachment 107150 [details]
Patch

Attachment 107150 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9651102
Comment 6 WebKit Review Bot 2011-09-13 02:07:10 PDT
Comment on attachment 107150 [details]
Patch

Attachment 107150 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9655050
Comment 7 Kent Tamura 2011-09-13 02:10:03 PDT
Oops, I didn't check Release build.
Comment 8 Kent Tamura 2011-09-13 02:25:19 PDT
Created attachment 107154 [details]
Patch 2

Fixed Release build
Comment 9 Simon Fraser (smfr) 2011-09-13 08:48:43 PDT
We'll need some performance measurements before accepting this patch.
Comment 10 Darin Adler 2011-09-13 09:42:12 PDT
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.
Comment 11 Kent Tamura 2011-09-14 01:14:07 PDT
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.
Comment 12 Kent Tamura 2011-09-14 01:36:08 PDT
(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%
Comment 13 Kent Tamura 2011-09-14 01:55:59 PDT
Created attachment 107308 [details]
Patch 3

Follows Darin's comments
Comment 14 Kent Tamura 2011-09-14 02:00:00 PDT
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 15 Ryosuke Niwa 2011-09-14 21:41:22 PDT
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.
Comment 16 Kent Tamura 2011-09-15 06:45:35 PDT
Simon, Darin, do you have further comments?
Comment 17 Darin Adler 2011-09-15 07:33:10 PDT
(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 18 Darin Adler 2011-09-15 07:35:12 PDT
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.
Comment 19 Kent Tamura 2011-09-15 17:33:34 PDT
(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 20 Kent Tamura 2011-09-20 21:30:41 PDT
Comment on attachment 107308 [details]
Patch 3

I'll land this patch as is.  Feel free to post another patch for objectIsRelayoutBoundary() :-)
Comment 21 WebKit Review Bot 2011-09-20 22:37:55 PDT
Comment on attachment 107308 [details]
Patch 3

Clearing flags on attachment: 107308

Committed r95598: <http://trac.webkit.org/changeset/95598>
Comment 22 WebKit Review Bot 2011-09-20 22:38:00 PDT
All reviewed patches have been landed.  Closing bug.