Bug 17257 – start ref counts at 1 instead of 0 for speed

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

We will change classes that do ref counting to start their ref counts at 1 instead of starting at 0 and then getting incremented to 1 by a later call to ref().

[Darin Adler]

Created attachment 19017 [details]
patch

Comment on attachment 19017 [details]
patch

r=me

+        (KJS::RegExp::create): Added. Calls new RegExp then adopt's the initial ref.

Can simply say "adopts" without the apostrophe.

[Darin Adler]

Comment on attachment 19017 [details]
patch

Clearing the review flag since I landed this.

[Darin Adler]

Created attachment 19212 [details]
patch for next step -- actually remove the default

[Anders Carlsson]

Comment on attachment 19212 [details]
patch for next step -- actually remove the default

r=me

[Darin Adler]

Comment on attachment 19212 [details]
patch for next step -- actually remove the default

Landed. Clearing review flag.

[Darin Adler]

Created attachment 19228 [details]
work in progress on StyleBase and classes derived from it

[Darin Adler]

Created attachment 19229 [details]
work in progress on some other RefCounted classes

[Darin Adler]

Created attachment 19230 [details]
a patch with a "sniffer" I was using to see 0 ref counts that were actually hit at runtime

[Darin Adler]

Comment on attachment 19017 [details]
patch

Marking patch already-landed as obsolete so the list of patches is easier to see.

[Darin Adler]

Comment on attachment 19212 [details]
patch for next step -- actually remove the default

Marking patch already-landed as obsolete so the list of patches is easier to see.

[Darin Adler]

Anders is working on this now (even more than I am). We're down to only 70 sites that use RefCounted(0).

[Darin Adler]

Created attachment 21561 [details]
patch

Comment on attachment 21561 [details]
patch

These look like they don't belong in this patch:

JavaScriptCore/VM/CodeBlock.cpp
WebCore/plugins/PluginDebug.h (also included in the ChangeLog)
WebKitTools/Scripts/make-js-test-wrappers

Other comments:

 38     PassRefPtr<StaticNodeList> createSelectorNodeList(PassRefPtr<Node> rootNode, CSSSelector*);

Maybe this should be a static StaticNodeList method? Perhaps StaticNodeList::create[By|With|From]Selector().

 101     : RefCounted<HistoryItem>(1)

Is the 1 necessary here?

 79     static PassRefPtr<HTMLCollection> create(PassRefPtr<Node> base, Type);

 38     static PassRefPtr<HTMLFormCollection> create(PassRefPtr<HTMLFormElement>);

 38     static PassRefPtr<HTMLOptionsCollection> create(PassRefPtr<HTMLSelectElement>);

 41     static PassRefPtr<HTMLTableRowsCollection> create(PassRefPtr<HTMLTableElement>);

Why are those not defined in the header?

I cannot r+ because I haven't read JSSVGTransformListCustom.cpp, JSSVGPointListCustom.cpp and JSSVGPODTypeWrapper.h. I will try to get to them later or somebody else could review just those (or you can land everything else with r=me).

Comment on attachment 21561 [details]
patch

These look like they don't belong in this patch:

JavaScriptCore/VM/CodeBlock.cpp
WebCore/plugins/PluginDebug.h (also included in the ChangeLog)
WebKitTools/Scripts/make-js-test-wrappers

Other comments:

 38     PassRefPtr<StaticNodeList> createSelectorNodeList(PassRefPtr<Node> rootNode, CSSSelector*);

Maybe this should be a static StaticNodeList method? Perhaps StaticNodeList::create[By|With|From]Selector().

 101     : RefCounted<HistoryItem>(1)

Is the 1 necessary here?

 79     static PassRefPtr<HTMLCollection> create(PassRefPtr<Node> base, Type);

 38     static PassRefPtr<HTMLFormCollection> create(PassRefPtr<HTMLFormElement>);

 38     static PassRefPtr<HTMLOptionsCollection> create(PassRefPtr<HTMLSelectElement>);

 41     static PassRefPtr<HTMLTableRowsCollection> create(PassRefPtr<HTMLTableElement>);

Why are those not defined in the header?

I cannot r+ because I haven't read JSSVGTransformListCustom.cpp, JSSVGPointListCustom.cpp and JSSVGPODTypeWrapper.h. I will try to get to them later or somebody else could review just those (or you can land everything else with r=me).

[Darin Adler]

(In reply to comment #14)
> These look like they don't belong in this patch:
> 
> JavaScriptCore/VM/CodeBlock.cpp

Oops. Need to land that to fix some warnings, but clearly not the same patch.

> WebCore/plugins/PluginDebug.h (also included in the ChangeLog)

Right, just another warning fix.

> WebKitTools/Scripts/make-js-test-wrappers

Yes, same thing.

> Other comments:
> 
>  38     PassRefPtr<StaticNodeList> createSelectorNodeList(PassRefPtr<Node>
> rootNode, CSSSelector*);
> 
> Maybe this should be a static StaticNodeList method? Perhaps
> StaticNodeList::create[By|With|From]Selector().

I don't see why it should be. This is a function that yields a static node list. I wouldn't want to make all functions that yield vectors members of the Vector class.

>  101     : RefCounted<HistoryItem>(1)
> 
> Is the 1 necessary here?

It is. This is a copy constructor and so we need to initialize base classes explicitly. I tried without it first.

>  79     static PassRefPtr<HTMLCollection> create(PassRefPtr<Node> base, Type);
>
>  38     static PassRefPtr<HTMLFormCollection>
> create(PassRefPtr<HTMLFormElement>);
> 
>  38     static PassRefPtr<HTMLOptionsCollection>
> create(PassRefPtr<HTMLSelectElement>);
> 
>  41     static PassRefPtr<HTMLTableRowsCollection>
> create(PassRefPtr<HTMLTableElement>);
> 
> Why are those not defined in the header?

Because I'd have to add additional includes to make those headers compile, and I don't think it's crucial that all these functions be inlined.

> I cannot r+ because I haven't read JSSVGTransformListCustom.cpp,
> JSSVGPointListCustom.cpp and JSSVGPODTypeWrapper.h. I will try to get to them
> later or somebody else could review just those (or you can land everything else
> with r=me).

I think I'll separate those into a different patch since you reviewed the rest.

[David Krause]

It looks like the latest patch breaks the Gtk build.  WebKit/gtk/webkit/webkitwebhistoryitem.cpp references an old version of the HistoryItem function that was removed.  

Specifically, the webkit_web_history_item_new_with_data is using the HistoryItem constructor that takes a KURL* and String as parameters.

[Darin Adler]

Comment on attachment 21561 [details]
patch

I landed the part of this that Dan reviewed, so clearing the review flag.

[Darin Adler]

Created attachment 21572 [details]
patch that does this for more classes

[Sam Weinig]

Comment on attachment 21572 [details]
patch that does this for more classes

r=me.

Because I like the one class per file rule, I would would prefer that SimpleEditCommand be in its own file, but, I realize that is a little odd for such a small file.

[Darin Adler]

Comment on attachment 21572 [details]
patch that does this for more classes

Clearing review flag since I landed this.

[Darin Adler]

Created attachment 21703 [details]
patch that deals with almost all remaining classes

After this patch is landed, what we have left is:

    StyleBase
    NodeFilter
    TreeShared
    ThreadSafeShared

[Sam Weinig]

Comment on attachment 21703 [details]
patch that deals with almost all remaining classes

Can ParserRefCounted::hasOneRef be const as well?

r=me

[Darin Adler]

(In reply to comment #23)
> Can ParserRefCounted::hasOneRef be const as well?

I tried making it const, ran into minor problems (casting to work with the dictionary), and so decided not to bother.

[Darin Adler]

Comment on attachment 21703 [details]
patch that deals with almost all remaining classes

Clearing review flag since this was landed.

[Darin Adler]

Created attachment 21722 [details]
patch that converts StyleBase and classes derived from it

[Sam Weinig]

Comment on attachment 21722 [details]
patch that converts StyleBase and classes derived from it

I think it would be better to make the computedStyle function be called CSSComputedStyleDeclaration::create.  Is there a reason to change the style for it?

Can this be done with a default value for  charset of String()?

+    static PassRefPtr<CSSStyleSheet> create(Node* ownerNode, const String& href)
+    {
+        return adoptRef(new CSSStyleSheet(ownerNode, href, String()));
+    }
+    static PassRefPtr<CSSStyleSheet> create(Node* ownerNode, const String& href, const String& charset)
+    {
+        return adoptRef(new CSSStyleSheet(ownerNode, href, charset));
+    }


r=me

[Darin Adler]

(In reply to comment #27)
> (From update of attachment 21722 [details] [edit])
> I think it would be better to make the computedStyle function be called
> CSSComputedStyleDeclaration::create.  Is there a reason to change the style for
> it?

The computedStyle function already existed. I didn't want to have both that function and CSSComputedStyleDeclaration::create. I could be convinced to do it another way.

> Can this be done with a default value for  charset of String()?
> 
> +    static PassRefPtr<CSSStyleSheet> create(Node* ownerNode, const String&
> href)
> +    {
> +        return adoptRef(new CSSStyleSheet(ownerNode, href, String()));
> +    }
> +    static PassRefPtr<CSSStyleSheet> create(Node* ownerNode, const String&
> href, const String& charset)
> +    {
> +        return adoptRef(new CSSStyleSheet(ownerNode, href, charset));
> +    }

Sure, it could be done either way. Those are just two different ways of writing the same code.

[Darin Adler]

Comment on attachment 21722 [details]
patch that converts StyleBase and classes derived from it

Clearing reviewed flag on this one since I landed it.

[Darin Adler]

(In reply to comment #22)
> After this patch is landed, what we have left is:
> 
>     StyleBase

Done.

>     NodeFilter

Done.

>     TreeShared

This one is really a separate task. When we wean classes away from TreeShared, we can make the refcount start at 1 at the same time.

>     ThreadSafeShared

I was mistaken, this was already done a while ago.