Bug 17257

Summary: start ref counts at 1 instead of 0 for speed
Product: WebKit Reporter: Darin Adler <darin>
Component: Web Template FrameworkAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, david.krause
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
patch for next step -- actually remove the default
none
work in progress on StyleBase and classes derived from it
none
work in progress on some other RefCounted classes
none
a patch with a "sniffer" I was using to see 0 ref counts that were actually hit at runtime
none
patch
none
patch that does this for more classes
none
patch that deals with almost all remaining classes
none
patch that converts StyleBase and classes derived from it none

Description Darin Adler 2008-02-09 07:04:35 PST
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().
Comment 1 Darin Adler 2008-02-09 07:05:45 PST
Created attachment 19017 [details]
patch
Comment 2 mitz 2008-02-09 08:56:46 PST
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.
Comment 3 Darin Adler 2008-02-09 10:10:10 PST
Comment on attachment 19017 [details]
patch

Clearing the review flag since I landed this.
Comment 4 Darin Adler 2008-02-19 10:57:52 PST
Created attachment 19212 [details]
patch for next step -- actually remove the default
Comment 5 Anders Carlsson 2008-02-19 10:59:02 PST
Comment on attachment 19212 [details]
patch for next step -- actually remove the default

r=me
Comment 6 Darin Adler 2008-02-19 11:07:22 PST
Comment on attachment 19212 [details]
patch for next step -- actually remove the default

Landed. Clearing review flag.
Comment 7 Darin Adler 2008-02-20 09:17:24 PST
Created attachment 19228 [details]
work in progress on StyleBase and classes derived from it
Comment 8 Darin Adler 2008-02-20 09:18:34 PST
Created attachment 19229 [details]
work in progress on some other RefCounted classes
Comment 9 Darin Adler 2008-02-20 09:19:20 PST
Created attachment 19230 [details]
a patch with a "sniffer" I was using to see 0 ref counts that were actually hit at runtime
Comment 10 Darin Adler 2008-02-24 02:35:18 PST
Comment on attachment 19017 [details]
patch

Marking patch already-landed as obsolete so the list of patches is easier to see.
Comment 11 Darin Adler 2008-02-24 02:35:28 PST
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.
Comment 12 Darin Adler 2008-06-05 09:36:13 PDT
Anders is working on this now (even more than I am). We're down to only 70 sites that use RefCounted(0).
Comment 13 Darin Adler 2008-06-07 09:45:04 PDT
Created attachment 21561 [details]
patch
Comment 14 mitz 2008-06-07 11:07:45 PDT
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 15 mitz 2008-06-07 11:07:45 PDT
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 16 Darin Adler 2008-06-07 13:49:05 PDT
(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.
Comment 17 David Krause 2008-06-07 17:10:38 PDT
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.
Comment 18 Darin Adler 2008-06-08 07:52:46 PDT
Comment on attachment 21561 [details]
patch

I landed the part of this that Dan reviewed, so clearing the review flag.
Comment 19 Darin Adler 2008-06-08 09:01:46 PDT
Created attachment 21572 [details]
patch that does this for more classes
Comment 20 Sam Weinig 2008-06-13 18:17:02 PDT
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.
Comment 21 Darin Adler 2008-06-14 17:44:58 PDT
Comment on attachment 21572 [details]
patch that does this for more classes

Clearing review flag since I landed this.
Comment 22 Darin Adler 2008-06-14 18:01:15 PDT
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
Comment 23 Sam Weinig 2008-06-14 18:30:30 PDT
Comment on attachment 21703 [details]
patch that deals with almost all remaining classes

Can ParserRefCounted::hasOneRef be const as well?

r=me
Comment 24 Darin Adler 2008-06-14 20:10:10 PDT
(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.
Comment 25 Darin Adler 2008-06-14 20:44:24 PDT
Comment on attachment 21703 [details]
patch that deals with almost all remaining classes

Clearing review flag since this was landed.
Comment 26 Darin Adler 2008-06-16 00:09:46 PDT
Created attachment 21722 [details]
patch that converts StyleBase and classes derived from it
Comment 27 Sam Weinig 2008-06-16 11:51:20 PDT
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
Comment 28 Darin Adler 2008-06-16 16:36:49 PDT
(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.
Comment 29 Darin Adler 2008-06-17 12:18:12 PDT
Comment on attachment 21722 [details]
patch that converts StyleBase and classes derived from it

Clearing reviewed flag on this one since I landed it.
Comment 30 Darin Adler 2008-06-17 22:36:06 PDT
(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.