Bug 103096

Summary:

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

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, andersca, dglazkov, ggaren, haraken, japhet, koivisto, loislo, ojan, peter+ews, sam, webkit.review.bot, yurys

Priority:

Keywords:

Regression

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

78909

Attachments:

Description	Flags
Fixes the bug	none
Fixed the typo: REGRESSIN	none
rniwa's patch with chromium compilation fix and native memory instrumentation fix	none
Put back inline comments	sam: review+

Ryosuke Niwa

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2012-11-22 20:55:59 PST

Created attachment 175731 [details] Fixes the bug

Ryosuke Niwa

Comment 2 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!

Ryosuke Niwa

Comment 3 2012-11-22 20:59:55 PST

Created attachment 175732 [details] Fixed the typo: REGRESSIN

Ryosuke Niwa

Comment 4 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?

Peter Beverloo (cr-android ews)

Comment 5 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

Ilya Tikhonovsky

Comment 6 2012-11-22 22:15:13 PST

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

Adam Barth

Comment 7 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.

Ilya Tikhonovsky

Comment 8 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.

Ilya Tikhonovsky

Comment 9 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?

Adam Barth

Comment 10 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.

Sam Weinig

Comment 11 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?

Ryosuke Niwa

Comment 12 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.

Ryosuke Niwa

Comment 13 2012-11-23 18:13:36 PST

Created attachment 175842 [details] Put back inline comments

Sam Weinig

Comment 14 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.

Ryosuke Niwa

Comment 15 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.

Sam Weinig

Comment 16 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.

Sam Weinig

Comment 17 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.

Ryosuke Niwa

Comment 18 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.

Ryosuke Niwa

Comment 19 2012-11-25 00:02:06 PST

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

Ojan Vafai

Comment 20 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

Ojan Vafai

Comment 21 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.

Note You need to log in before you can comment on or make changes to this bug.