Bug 69569 – Remove colSpan / rowSpan caching from RenderTableCell

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Julien Chaffraix]

Currently those 2 fields are used during layout and to check that colSpan or rowSpan have changed in updateElement. However most of this use does not mandate having 2 integers stored per cells. Especially since HTMLTableElement already stores them.

[Julien Chaffraix]

Created attachment 110047 [details]
Proposed change: Removed more caching of colSpan / rowSpan.

[Darin Adler]

Comment on attachment 110047 [details]
Proposed change: Removed more caching of colSpan / rowSpan.

View in context: https://bugs.webkit.org/attachment.cgi?id=110047&action=review

> Source/WebCore/html/HTMLTableCellElement.cpp:97
> +        int oldRowSpan = max(1, attr->value().toInt());

Is the value in Attribute* really the old value? Are you sure?

[Darin Adler]

Comment on attachment 110047 [details]
Proposed change: Removed more caching of colSpan / rowSpan.

How did you measure the performance impact of this change?

[Julien Chaffraix]

> > Source/WebCore/html/HTMLTableCellElement.cpp:97
> > +        int oldRowSpan = max(1, attr->value().toInt());
> 
> Is the value in Attribute* really the old value? Are you sure?

I double checked and you are right. I was confused by Element::setAttribute who calls the attribute |old| but then sets the new value on it. The check is always false (I am waiting on the cr-linux EWS if it does not complain I will open a bug about colSpan / rowSpan mutation needing some coverage).

> How did you measure the performance impact of this change?

Not that it matters but I measured using PageCycler to see the impact on real pages. I did not want to go with a micro-benchmark if that's the underlying question.

[WebKit Review Bot]

Comment on attachment 110047 [details]
Proposed change: Removed more caching of colSpan / rowSpan.

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

New failing tests:
fast/dom/HTMLTableElement/colSpan.html
tables/mozilla/bugs/bug22246-2a.html
tables/mozilla/bugs/bug22246-3a.html
tables/mozilla/bugs/bug22246-3.html
tables/mozilla/bugs/bug22246-2.html

[Julien Chaffraix]

Comment on attachment 110047 [details]
Proposed change: Removed more caching of colSpan / rowSpan.

As pointed out by Darin, this patch is horribly wrong. I will see if some of the changes can be extracted - like removing the virtual dispatch - but I doubt we can easily save the memory without an unneeded overhaul of the way we handle attribute change.

[Julien Chaffraix]

I looked into the performance degradation linked to this patch when getting rowSpan or colSpan - using a micro-benchmark - and couldn't get a significant measure (it looked around 1.5% degradation but that fell within the statistical noise).

The patch saves 4% memory (8 bytes out of 208 bytes for a RenderTableCell on my machine) and could save 4% more if we are manage to pack 2 bits. However for the change to work, we would need to remove the old vs new value check.

Looking at Chrome's benchmark pages (Alexia US among them), rowSpan / colSpan don't seem to be changed or queried through JS and are set once inside HTML. IMHO it would be fine to go ahead and save the memory but I would welcome comments from other developers.

[Darin Adler]

Seems OK to me to remove the caching. Seems we should have some way in the attribute change mechanism to consider both the old and new values of an attribute when responding to a change without having to pay memory to store the old value.

[Julien Chaffraix]

(In reply to comment #8)
> Seems OK to me to remove the caching. Seems we should have some way in the attribute change mechanism to consider both the old and new values of an attribute when responding to a change without having to pay memory to store the old value.

That's definitely a good idea. I looked at some of the classes in html/ to see how often we cache the attribute: in some case, we store the attribute to avoid reparsing but most of the cases are just to detect attribute change (and also caching). Filed bug 70177 to cover adding a way of getting the old value.

[Julien Chaffraix]

Created attachment 111138 [details]
Change 2: Removed the bogus span changed logic.

[Darin Adler]

Comment on attachment 111138 [details]
Change 2: Removed the bogus span changed logic.

View in context: https://bugs.webkit.org/attachment.cgi?id=111138&action=review

> Source/WebCore/ChangeLog:14
> +        on PageCycler Alexia-US.

I think it’s Alexa, not Alexia.

> Source/WebCore/html/HTMLTableCellElement.h:29
> +#include "HTMLNames.h"

When I did this recently in some other classes, I avoided putting HTMLNames.h into the header by putting the assertions into the .cpp file and having only the NDEBUG version be inlined.

> Source/WebCore/rendering/RenderTableCell.cpp:52
> +    m_hasAssociatedTableCellElement = node && (node->hasTagName(tdTag) || node->hasTagName(thTag));

Seems this should be done with initialization, not assignment.

> Source/WebCore/rendering/RenderTableCell.cpp:84
> +    ASSERT(node() && (node()->hasTagName(tdTag) || node()->hasTagName(thTag)));

Normally we do not use && in ASSERT. Instead we do two separate assertions. That allows us to see which has failed.

> Source/WebCore/rendering/RenderTableCell.cpp:87
> +    if (parent() && section())

I’m not sure the null check of parent() is helpful, even though the old code had it.

> Source/WebCore/rendering/RenderTableCell.h:153
> +    // FIXME: It would be nice to pack those 2 bits into some of the previous fields.

Should be "these", not "those".

[Julien Chaffraix]

Comment on attachment 111138 [details]
Change 2: Removed the bogus span changed logic.

View in context: https://bugs.webkit.org/attachment.cgi?id=111138&action=review

>> Source/WebCore/rendering/RenderTableCell.cpp:87
>> +    if (parent() && section())
> 
> I’m not sure the null check of parent() is helpful, even though the old code had it.

I am pretty sure it is needed. Calling section() assumes the following hierarchy: RenderTableSection -> RenderTableRow -> RenderTableCell. However nothing guarantees that it is respected and section() does not check for parent() so you could end up dereferencing NULL.

[Julien Chaffraix]

Created attachment 111277 [details]
Patch for landing

[Julien Chaffraix]

Committed r97691: <http://trac.webkit.org/changeset/97691>