Bug 17257 - start ref counts at 1 instead of 0 for speed
Summary: start ref counts at 1 instead of 0 for speed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-02-09 07:04 PST by Darin Adler
Modified: 2008-06-17 22:36 PDT (History)
2 users (show)

See Also:


Attachments
patch (11.04 KB, patch)
2008-02-09 07:05 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch for next step -- actually remove the default (1.24 KB, patch)
2008-02-19 10:57 PST, Darin Adler
no flags Details | Formatted Diff | Diff
work in progress on StyleBase and classes derived from it (66.76 KB, patch)
2008-02-20 09:17 PST, Darin Adler
no flags Details | Formatted Diff | Diff
work in progress on some other RefCounted classes (70.45 KB, patch)
2008-02-20 09:18 PST, Darin Adler
no flags Details | Formatted Diff | Diff
a patch with a "sniffer" I was using to see 0 ref counts that were actually hit at runtime (1.25 KB, patch)
2008-02-20 09:19 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch (159.95 KB, patch)
2008-06-07 09:45 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch that does this for more classes (293.36 KB, patch)
2008-06-08 09:01 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch that deals with almost all remaining classes (98.22 KB, patch)
2008-06-14 18:01 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch that converts StyleBase and classes derived from it (270.63 KB, patch)
2008-06-16 00:09 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.