Bug 67989 – Move RenderObject::markContainingBlocksForLayout() to RenderObject.cpp

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 107150 [details]
Patch

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 107150 [details]
Patch

Attachment 107150 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9654054

[Gyuyoung Kim]

Comment on attachment 107150 [details]
Patch

Attachment 107150 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9660046

[WebKit Review Bot]

Comment on attachment 107150 [details]
Patch

Attachment 107150 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9651102

[WebKit Review Bot]

Comment on attachment 107150 [details]
Patch

Attachment 107150 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9655050

[Kent Tamura]

Oops, I didn't check Release build.

[Kent Tamura]

Created attachment 107154 [details]
Patch 2

Fixed Release build

[Simon Fraser (smfr)]

We'll need some performance measurements before accepting this patch.

[Darin Adler]

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.

[Kent Tamura]

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.

[Kent Tamura]

(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%

[Kent Tamura]

Created attachment 107308 [details]
Patch 3

Follows Darin's comments

[Kent Tamura]

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.'

[Ryosuke Niwa]

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.

[Kent Tamura]

Simon, Darin, do you have further comments?

[Darin Adler]

(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.

[Darin Adler]

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.

[Kent Tamura]

(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.

[Kent Tamura]

Comment on attachment 107308 [details]
Patch 3

I'll land this patch as is.  Feel free to post another patch for objectIsRelayoutBoundary() :-)

[WebKit Review Bot]

Comment on attachment 107308 [details]
Patch 3

Clearing flags on attachment: 107308

Committed r95598: <http://trac.webkit.org/changeset/95598>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.