Bug 103096 – REGRESSION(r135493): HTMLCollection and DynamicNodeList have two vtable pointers

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

http://trac.webkit.org/changeset/135493 increased the size of each DynamicNodeList and HTMLCollection by 4 to 8 bytes (on 32-bit and 64-bit builds respectively) by adding a new vtable pointer to DynamicNodeListCacheBase.

[Ryosuke Niwa]

Created attachment 175731 [details]
Fixes the bug

[Ryosuke Niwa]

This should be your favorite patch of the day. It reduces memory usage by removing multiple inheritance and removing redundant code!

[Ryosuke Niwa]

Created attachment 175732 [details]
Fixed the typo: REGRESSIN

[Ryosuke Niwa]

Comment on attachment 175732 [details]
Fixed the typo: REGRESSIN

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

> Source/WebCore/dom/DynamicNodeList.h:71
>      virtual void reportMemoryUsage(MemoryObjectInfo*) const;

I wonder if I can get rid of this function now. Any ideas?

[Peter Beverloo (cr-android ews)]

Comment on attachment 175732 [details]
Fixed the typo: REGRESSIN

Attachment 175732 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14965569

[Ilya Tikhonovsky]

Created attachment 175736 [details]
rniwa's patch with chromium compilation fix and native memory instrumentation fix

[Adam Barth]

Comment on attachment 175736 [details]
rniwa's patch with chromium compilation fix and native memory instrumentation fix

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

> Source/WebCore/dom/DynamicNodeList.h:44
> -class DynamicNodeListCacheBase {
> +class DynamicNodeListCacheBase : public NodeList {

Now HTMLCollection has two copies of ScriptWrappable as a base clase: one from NodeList and one from HTMLCollection.  You can just remove the one in HTMLCollection.

[Ilya Tikhonovsky]

Comment on attachment 175736 [details]
rniwa's patch with chromium compilation fix and native memory instrumentation fix

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

>> Source/WebCore/dom/DynamicNodeList.h:44
>> +class DynamicNodeListCacheBase : public NodeList {
> 
> Now HTMLCollection has two copies of ScriptWrappable as a base clase: one from NodeList and one from HTMLCollection.  You can just remove the one in HTMLCollection.

As far as I see rniwa has fixed this problem in this patch.

[Ilya Tikhonovsky]

(In reply to comment #6)
> Created an attachment (id=175736) [details]
> rniwa's patch with chromium compilation fix and native memory instrumentation fix

As far as I see rniwa has fixed this problem in this patch.
See HTMLCollection.h:36

r?

[Adam Barth]

Comment on attachment 175736 [details]
rniwa's patch with chromium compilation fix and native memory instrumentation fix

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

> Source/WebCore/html/HTMLCollection.h:36
> -class HTMLCollection : public ScriptWrappable, public RefCounted<HTMLCollection>, public DynamicNodeListCacheBase {
> +class HTMLCollection : public DynamicNodeListCacheBase {

Ah, great!  I'm not sure how I missed this part of the change earlier.

[Sam Weinig]

I don't love DynamicNodeListCacheBase inheriting from NodeList.  If it is going to be an isa relationship, can we find a better name for DynamicNodeListCacheBase?  Maybe just DynamicNodeListBase?

[Ryosuke Niwa]

(In reply to comment #11)
> I don't love DynamicNodeListCacheBase inheriting from NodeList.  If it is going to be an isa relationship, can we find a better name for DynamicNodeListCacheBase?  Maybe just DynamicNodeListBase?

Yeah, I'm planning to do renames in a follow up. For starters, we should rename DynamicNodeList to LiveNodeList to match the terminologies in specifications.

[Ryosuke Niwa]

Created attachment 175842 [details]
Put back inline comments

[Sam Weinig]

(In reply to comment #12)
> (In reply to comment #11)
> > I don't love DynamicNodeListCacheBase inheriting from NodeList.  If it is going to be an isa relationship, can we find a better name for DynamicNodeListCacheBase?  Maybe just DynamicNodeListBase?
> 
> Yeah, I'm planning to do renames in a follow up. For starters, we should rename DynamicNodeList to LiveNodeList to match the terminologies in specifications.

I always liked DynamicNodeList (since it is opposed to StaticNodeList).  But my real issue is with the CacheBase part.

[Ryosuke Niwa]

r(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > I don't love DynamicNodeListCacheBase inheriting from NodeList.  If it is going to be an isa relationship, can we find a better name for DynamicNodeListCacheBase?  Maybe just DynamicNodeListBase?
> > 
> > Yeah, I'm planning to do renames in a follow up. For starters, we should rename DynamicNodeList to LiveNodeList to match the terminologies in specifications.
> 
> I always liked DynamicNodeList (since it is opposed to StaticNodeList).  But my real issue is with the CacheBase part.

Sure, of course. Do you want me to do the rename in this patch? I was planning to do in a separate patch since it'll make the diff messier but if you strongly feel that we should do the rename in the same patch, I can do that as well.

[Sam Weinig]

(In reply to comment #15)
> r(In reply to comment #14)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > I don't love DynamicNodeListCacheBase inheriting from NodeList.  If it is going to be an isa relationship, can we find a better name for DynamicNodeListCacheBase?  Maybe just DynamicNodeListBase?
> > > 
> > > Yeah, I'm planning to do renames in a follow up. For starters, we should rename DynamicNodeList to LiveNodeList to match the terminologies in specifications.
> > 
> > I always liked DynamicNodeList (since it is opposed to StaticNodeList).  But my real issue is with the CacheBase part.
> 
> Sure, of course. Do you want me to do the rename in this patch? I was planning to do in a separate patch since it'll make the diff messier but if you strongly feel that we should do the rename in the same patch, I can do that as well.

No, I don't this should be blocked on it.

[Sam Weinig]

Comment on attachment 175842 [details]
Put back inline comments

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

> Source/WebCore/dom/DynamicNodeList.h:45
> +class DynamicNodeListCacheBase : public NodeList {
>  public:

Please do add a FIXME about how horrible a name this is now.

[Ryosuke Niwa]

Comment on attachment 175842 [details]
Put back inline comments

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

>> Source/WebCore/dom/DynamicNodeList.h:45
>>  public:
> 
> Please do add a FIXME about how horrible a name this is now.

Will do. Thanks.

[Ryosuke Niwa]

Committed r135667: <http://trac.webkit.org/changeset/135667>

[Ojan Vafai]

FWIW, this fix also had a ~7% improvement for some of the Chromium perf tests:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/intl1/report.html?history=50&rev=169445

[Ojan Vafai]

(In reply to comment #20)
> FWIW, this fix also had a ~7% improvement for some of the Chromium perf tests:
> http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/intl1/report.html?history=50&rev=169445

Whoops, nevermind. It was 135666.