Smaller vtbls in RenderObject

Smaller vtbls in RenderObject.

There are 101 different classes in the RenderObject class tree and
they share 150-250 virtual functions each. That scales to 60-200 KB
of vtbls (depending on architecture), which cost space in the
binary and time during startup since they need to be copied 
to RAM and adjusted. In fact, some RenderObject vtbls are the
largest vtbls in Chrome.

There doesn't have to be anything wrong with any of this, but the
use of 60+ virtual isXXX functions which were the same for 100 out of
101 classes could easily be replaced by a single virtual function, saving 20-50 KB of the space mentioned above (again depending on architecture).

When the class is known, the isOfType functions will constant fold, just
like the isFoo methods.

clang x64 space info:
Total change: -47075 bytes
==========================
  78 added, totalling +4731 bytes across 70 sources
  142 removed, totalling -4008 bytes across 70 sources
  397 grown, for a net change of +5064 bytes (296097 bytes before, 301161 bytes after) across 128 sources
  115 shrunk, for a net change of -52862 bytes (209130 bytes before, 156268 bytes after) across 5 sources

gcc x64 space into:
Total change: -49078 bytes
==========================
  141 added, totalling +19000 bytes across 90 sources
  200 removed, totalling -21292 bytes across 90 sources
  371 grown, for a net change of +17491 bytes (387125 bytes before, 404616 bytes after) across 164 sources
  261 shrunk, for a net change of -64277 bytes (387358 bytes before, 323081 bytes after) across 98 sources

R=esprehn@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183832

[Daniel Bratell, 2014-09-30 09:20:15]

Patchset #1 (id:1) has been deleted

[Daniel Bratell, 2014-10-01 07:49:45]

bratell@opera.com changed reviewers:
+ esprehn@chromium.org

[Daniel Bratell, 2014-10-01 07:49:47]

https://codereview.chromium.org/613783002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/613783002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:1132: RenderObjectSVG, /* Keep by itself?
*/
Comment need to go. The question was whether isSVG() should be kept as a
function of itself or not. I think it works this way.

[Daniel Bratell, 2014-10-01 07:50:04]

Patchset #1 (id:20001) has been deleted

[esprehn, 2014-10-01 08:05:48]

There's no reason to delete old patchsets unless you have a password or
something bad in there. I don't know why Rietveld surfaces that UI so
prominently...

Why do you call the super class method in each of these, do any superclasses do
anything? It seems like you only did this for leaf classes, why not change
isRenderBoxModelObject?

My concern with this is that now the isFoo() functions contain several branches
and a comparison and that which ones to do this pattern with is very arbitrary.
Can you run some perf tests?

[Daniel Bratell, 2014-10-01 09:28:41]

On 2014/10/01 08:05:48, esprehn wrote:
> There's no reason to delete old patchsets unless you have a password or
> something bad in there. I don't know why Rietveld surfaces that UI so
> prominently...
> 
> Why do you call the super class method in each of these, do any superclasses
do
> anything? It seems like you only did this for leaf classes, why not change
> isRenderBoxModelObject?
> 
> My concern with this is that now the isFoo() functions contain several
branches
> and a comparison and that which ones to do this pattern with is very
arbitrary.
> Can you run some perf tests?

Calling the base class is in case the base class will claim to be something as
well. It happens in SVG Render Objects and for the media/video objects but not
in most other classes so most of the time the whole call expands to nothing. On
the other hand, not doing it could lead to future bugs in case someone adds an
isOfType to one of the mid-level classes.

The old code (when not constant folded) was 
  400950:       31 c0                   xor    %eax,%eax
  400952:       c3                      retq   

The new code (when not constant folded) is
  400960:       83 ff 09                cmp    $0x9,%edi
  400963:       0f 94 c0                sete   %al
  400966:       c3                      retq   

Or in the SVG classes
  400970:       83 ff 09                cmp    $0x9,%edi
  400973:       0f 94 c0                sete   %al
  400976:       83 ff 07                cmp    $0x7,%edi
  400979:       0f 94 c2                sete   %dl
  40097c:       09 d0                   or     %edx,%eax
  40097e:       c3                      retq   

There are no branches or jumps except for in the calling code, and in the vtbl
dispatch and those would already be there. On the other hand there will be fewer
cache misses when accessing RenderObject vtbls since they will be smaller and
that will result in a performance increase. Also, since all isFoo will access
the same isOfType function for an object, there will be less need to access the
vtbl. Both the performance cost and performance increase I expect to be below
the noise level. I will try to produce some numbers from the performance layout
tests though.

[esprehn, 2014-10-01 10:30:04]

Why didn't you do the same thing for isRenderBoxModelObject though?

[Daniel Bratell, 2014-10-01 13:03:07]

Executive summary: Performance results show no change.

Performance results (ran base and with change 3 times each to get as much data
as possible). I've primarily compared the fastest run for each build with the
rationale that the code determines the fastest possible execution while anything
slower might be a background task, a busy memory bus or something else. Of
course turbo boost on modern cpus partially invalidates that assumption, but no
other interpretation of the data gives more obvious results.

Summary since this is a long and boring list:
All tests except two are obviously well within noise levels (of those 19 were
unlucky with noise and ended up slower and 14 were lucky with noise and ended up
faster).
The two remaining tests were one faster and one slower. Looking at the rest of
the data points it is not possible to say anything without yet many more data
points, but I think they are noise as well.

(minus is faster, plus is slower)
-1%   ArabicLineLayout (within noise levels)
+2%   MultipleShapes (within noise levels)
+1%   ShapeOutsideContentBox (within noise levels)
+0.5% ShapeOutsideInset (within noise levels)
+1%   ShapeOutsidePolygonWithMargin (within noise levels)
-0.5% ShapeOutsideRaster (within noise levels): 0.5% faster
-0.3% ShapeOutsideRasterWithMargin (within noise levels)
+0.2% ShapeOutsideSVGWithMargin (within noise levels)
+0.2% ShapeOutsideSimplePolygon (within noise levels)
+2%   ShapeOutsideStackedPolygons (within noise levels)
+4%   SimpleTextPathLineLayout (not obviously noise)
 0    auto-grid-lots-of-data
+0.2% chapter-reflow-once-random (within noise levels)
 0    chapter-reflow-once
+1%   chapter-reflow-thrice (within noise levels)
+0.3% chapter-reflow-twice (within noise levels)
+0.5% chapter-reflow (within noise levels) - interesting performance
characteristics
N/A   character_fallback (test results vary between 50 and 180 with or without
the patch)
-2%   fixed-grid-lots-of-data (within noise levels)
-2%   flexbox-column-nowrap (within noise levels)
-0.5% flexbox-column-wrap (within noise levels)
-1%   flexbox-row-nowrap (within noise levels)
-2%   flexbox-row-wrap (within noise levels)
 0    flexbox-with-stretch-layout
+1%   floats_100_100 (within noise levels)
+0.2% floats_100_100_nested (within noise levels)
-1%   floats_10_1000 (within noise levels)
+0.5% floats_20_100 (within noise levels)
 0    floats_20_100_nested
-4%   floats_2_100 (not obviously noise)
-0.5% floats_2_100_nested (within noise levels)
+1.5% floats_50_100 (within noise levels)
+0.5% floats_50_100_nested (within noise levels)
+0.5% hindi-line-layout (within noise levels)
-2%   large-table-with-collapsed-borders-and-colspans-wider-than-table (within
noise levels)
-1.5% large-table-with-collapsed-borders-and-colspans (within noise levels)
-2%   large-table-with-collapsed-borders-and-no-colspans (within noise levels)
+0.5% layers_overlap_2d (within noise levels)
 0    layers_overlap_3d
-1%   line-layout-line-height
+0.2% line-layout

[Daniel Bratell, 2014-10-01 13:12:40]

On 2014/10/01 10:30:04, esprehn wrote:
> Why didn't you do the same thing for isRenderBoxModelObject though?

I actually missed isRenderBoxModelObject (being before IsBR in the list, and I
started this with IsBR :-) ), but looking at it now I still wouldn't include it.

Of the 101 classes, it looks like almost all are RenderBoxModelObjects which
means that adding it to the IsOfType chain would expand all the checks with an
extra comparison. That makes such a change too close to the grey area where I
cannot say if it is good or bad, and where it might depend on architecture which
is the best solution for performance and size, thus I rather leave as is.

I don't rule out that it is possible to tweak this further, but we would be
discussing bytes and not KB. This patch also moves the RenderObject class tree
down from the position of being "most obviously using too much vtbl space" so
other things become more interesting if studying vtbls (like the AXObject class
tree, or the Element class tree).

[Daniel Bratell, 2014-10-01 13:12:48]

https://codereview.chromium.org/613783002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/613783002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.h:349: virtual bool isBoxModelObject() const
{ return false; }
Move isBoxModelObject to the other meta checks.

[Daniel Bratell, 2014-10-07 15:24:44]

esprehn, I think I responded to your questions above. What do you think?

[Daniel Bratell, 2014-10-16 16:22:32]

esprehn, can you please take a look? I've just rebased it to a world where
OVERRIDE is override so it will apply as is.

[esprehn, 2014-10-16 16:28:21]

This does mean there's a comparison inside these isFoo() functions now instead
of just a virtual call which makes me a little nervous but I guess lets see how
it goes.

lgtm

[Daniel Bratell, 2014-10-16 17:35:07]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2014-10-16 17:36:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613783002/80001

[Daniel Bratell, 2014-10-16 17:36:47]

On 2014/10/16 16:28:21, esprehn wrote:
> This does mean there's a comparison inside these isFoo() functions now instead
> of just a virtual call which makes me a little nervous but I guess lets see
how
> it goes.
> 
> lgtm

Thanks!

I think this is more well analyzed than most changes so I don't think there is a
reason to be more nervous about this than any other change to core.

[commit-bot: I haz the power, 2014-10-16 17:47:39]

Committed patchset #3 (id:80001) as 183832