New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 638104 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

devirt-want: LayoutObject::virtualChildren

Project Member Reported by krasin@chromium.org, Aug 16 2016

Issue description

LayoutObject::virtualChildren is an interesting beast. It comes in two flavours, const and not const, with the default implementation being zeroes:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutObject.h?cl=GROK&gsn=children&rcl=1471276009&l=262

virtual LayoutObjectChildList* virtualChildren() { return nullptr; }
virtual const LayoutObjectChildList* virtualChildren() const { return nullptr; }

All other overrides are *literally* the same:

LayoutObjectChildList* virtualChildren() final { return children(); }
const LayoutObjectChildList* virtualChildren() const final { return children(); }

With children() being a non-virtual instance method that is also literally the same:

const LayoutObjectChildList* children() const { return &m_children; }
LayoutObjectChildList* children() { return &m_children; }

And m_children is the same in all the classes which override virtualChildren:

LayoutObjectChildList m_children;

The motivation for this is described in the comment:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutObject.h?cl=GROK&gsn=children&rcl=1471276009&l=163

// To save memory, especially for the common child class LayoutText, LayoutObject doesn't provide
// storage for children. Descendant classes that do allow children have to have a LayoutObjectChildList
// member that stores the actual children and override virtualChildren().

It feels that this can be devirtualized by possibly making the offset of m_children field in all the classes the same.

Leaving this request for the future in the case it will prove to be really necessary, as the effort to devirtualize this is going to be rather high.
 

Comment 1 by krasin@chromium.org, Aug 16 2016

LayoutObject::canHaveChildren is literally just calling virtualChildren and therefore can be devirtualized.

Note: canHaveChildren can be devirtualized by virtual const prop, if instead of having the default implementation that depends on the data:

virtual bool canHaveChildren() const { return virtualChildren(); }

it would have more leaf overrides like:

bool canHaveChildren() const final { return true; }
or
bool canHaveChildren() const final { return false; }

The only annoying part is LayoutTableCol::canHaveChildren:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutTableCol.cpp?q=LayoutTableCol::canHaveChildren&sq=package:chromium&l=109&dr=CSs

bool LayoutTableCol::canHaveChildren() const
{
    // Cols cannot have children. This is actually necessary to fix a bug
    // with libraries.uc.edu, which makes a <p> be a table-column.
    return isTableColumnGroup();
}

where isTableColumnGroup has a relatively complex implementation:
bool isTableColumnGroup() const { return style()->display() == TABLE_COLUMN_GROUP; }

This still can be an "almost" virtual-const-prop, where it uses the value from vtable from all types but one, for which it calls the non-virtual method (in this case, isTableColumnGroup)
Status: Assigned (was: Untriaged)

Sign in to add a comment