Bug 103096 - REGRESSION(r135493): HTMLCollection and DynamicNodeList have two vtable pointers
Summary: REGRESSION(r135493): HTMLCollection and DynamicNodeList have two vtable pointers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: Regression
Depends on:
Blocks: 78909
  Show dependency treegraph
 
Reported: 2012-11-22 16:48 PST by Ryosuke Niwa
Modified: 2012-11-26 14:32 PST (History)
13 users (show)

See Also:


Attachments
Fixes the bug (13.85 KB, patch)
2012-11-22 20:55 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed the typo: REGRESSIN (13.85 KB, patch)
2012-11-22 20:59 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
rniwa's patch with chromium compilation fix and native memory instrumentation fix (16.64 KB, patch)
2012-11-22 22:15 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Put back inline comments (16.92 KB, patch)
2012-11-23 18:13 PST, Ryosuke Niwa
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-11-22 16:48:52 PST
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.
Comment 1 Ryosuke Niwa 2012-11-22 20:55:59 PST
Created attachment 175731 [details]
Fixes the bug
Comment 2 Ryosuke Niwa 2012-11-22 20:58:32 PST
This should be your favorite patch of the day. It reduces memory usage by removing multiple inheritance and removing redundant code!
Comment 3 Ryosuke Niwa 2012-11-22 20:59:55 PST
Created attachment 175732 [details]
Fixed the typo: REGRESSIN
Comment 4 Ryosuke Niwa 2012-11-22 21:05:56 PST
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?
Comment 5 Peter Beverloo (cr-android ews) 2012-11-22 21:36:50 PST
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
Comment 6 Ilya Tikhonovsky 2012-11-22 22:15:13 PST
Created attachment 175736 [details]
rniwa's patch with chromium compilation fix and native memory instrumentation fix
Comment 7 Adam Barth 2012-11-22 23:01:22 PST
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.
Comment 8 Ilya Tikhonovsky 2012-11-22 23:05:38 PST
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.
Comment 9 Ilya Tikhonovsky 2012-11-22 23:11:11 PST
(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?
Comment 10 Adam Barth 2012-11-22 23:13:01 PST
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.
Comment 11 Sam Weinig 2012-11-23 14:26:23 PST
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?
Comment 12 Ryosuke Niwa 2012-11-23 17:56:04 PST
(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.
Comment 13 Ryosuke Niwa 2012-11-23 18:13:36 PST
Created attachment 175842 [details]
Put back inline comments
Comment 14 Sam Weinig 2012-11-23 22:53:00 PST
(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.
Comment 15 Ryosuke Niwa 2012-11-24 17:15:09 PST
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.
Comment 16 Sam Weinig 2012-11-24 18:19:14 PST
(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.
Comment 17 Sam Weinig 2012-11-24 18:20:16 PST
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.
Comment 18 Ryosuke Niwa 2012-11-24 23:22:22 PST
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.
Comment 19 Ryosuke Niwa 2012-11-25 00:02:06 PST
Committed r135667: <http://trac.webkit.org/changeset/135667>
Comment 20 Ojan Vafai 2012-11-26 14:11:37 PST
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
Comment 21 Ojan Vafai 2012-11-26 14:32:05 PST
(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.