Bug 17257 - start ref counts at 1 instead of 0 for speed
: start ref counts at 1 instead of 0 for speed
Status: RESOLVED FIXED
: WebKit
Web Template Framework
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-02-09 07:04 PST by
Modified: 2008-06-17 22:36 PST (History)


Attachments
patch (11.04 KB, patch)
2008-02-09 07:05 PST, Darin Adler
no flags Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
patch (159.95 KB, patch)
2008-06-07 09:45 PST, Darin Adler
no flags Review Patch | Details | Formatted Diff | Diff
patch that does this for more classes (293.36 KB, patch)
2008-06-08 09:01 PST, Darin Adler
no flags Review Patch | Details | Formatted Diff | Diff
patch that deals with almost all remaining classes (98.22 KB, patch)
2008-06-14 18:01 PST, Darin Adler
no flags Review Patch | Details | Formatted Diff | Diff
patch that converts StyleBase and classes derived from it (270.63 KB, patch)
2008-06-16 00:09 PST, Darin Adler
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 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 From 2008-02-09 07:05:45 PST -------
Created an attachment (id=19017) [details]
patch
------- Comment #2 From 2008-02-09 08:56:46 PST -------
(From update of attachment 19017 [details])
r=me

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

Can simply say "adopts" without the apostrophe.
------- Comment #3 From 2008-02-09 10:10:10 PST -------
(From update of attachment 19017 [details])
Clearing the review flag since I landed this.
------- Comment #4 From 2008-02-19 10:57:52 PST -------
Created an attachment (id=19212) [details]
patch for next step -- actually remove the default
------- Comment #5 From 2008-02-19 10:59:02 PST -------
(From update of attachment 19212 [details])
r=me
------- Comment #6 From 2008-02-19 11:07:22 PST -------
(From update of attachment 19212 [details])
Landed. Clearing review flag.
------- Comment #7 From 2008-02-20 09:17:24 PST -------
Created an attachment (id=19228) [details]
work in progress on StyleBase and classes derived from it
------- Comment #8 From 2008-02-20 09:18:34 PST -------
Created an attachment (id=19229) [details]
work in progress on some other RefCounted classes
------- Comment #9 From 2008-02-20 09:19:20 PST -------
Created an attachment (id=19230) [details]
a patch with a "sniffer" I was using to see 0 ref counts that were actually hit at runtime
------- Comment #10 From 2008-02-24 02:35:18 PST -------
(From update of attachment 19017 [details])
Marking patch already-landed as obsolete so the list of patches is easier to see.
------- Comment #11 From 2008-02-24 02:35:28 PST -------
(From update of attachment 19212 [details])
Marking patch already-landed as obsolete so the list of patches is easier to see.
------- Comment #12 From 2008-06-05 09:36:13 PST -------
Anders is working on this now (even more than I am). We're down to only 70 sites that use RefCounted(0).
------- Comment #13 From 2008-06-07 09:45:04 PST -------
Created an attachment (id=21561) [details]
patch
------- Comment #14 From 2008-06-07 11:07:45 PST -------
(From update of attachment 21561 [details])
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 From 2008-06-07 11:07:45 PST -------
(From update of attachment 21561 [details])
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 From 2008-06-07 13:49:05 PST -------
(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 From 2008-06-07 17:10:38 PST -------
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 From 2008-06-08 07:52:46 PST -------
(From update of attachment 21561 [details])
I landed the part of this that Dan reviewed, so clearing the review flag.
------- Comment #19 From 2008-06-08 09:01:46 PST -------
Created an attachment (id=21572) [details]
patch that does this for more classes
------- Comment #20 From 2008-06-13 18:17:02 PST -------
(From update of attachment 21572 [details])
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 From 2008-06-14 17:44:58 PST -------
(From update of attachment 21572 [details])
Clearing review flag since I landed this.
------- Comment #22 From 2008-06-14 18:01:15 PST -------
Created an attachment (id=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 From 2008-06-14 18:30:30 PST -------
(From update of attachment 21703 [details])
Can ParserRefCounted::hasOneRef be const as well?

r=me 
------- Comment #24 From 2008-06-14 20:10:10 PST -------
(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 From 2008-06-14 20:44:24 PST -------
(From update of attachment 21703 [details])
Clearing review flag since this was landed.
------- Comment #26 From 2008-06-16 00:09:46 PST -------
Created an attachment (id=21722) [details]
patch that converts StyleBase and classes derived from it
------- Comment #27 From 2008-06-16 11:51:20 PST -------
(From update of attachment 21722 [details])
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 From 2008-06-16 16:36:49 PST -------
(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 From 2008-06-17 12:18:12 PST -------
(From update of attachment 21722 [details])
Clearing reviewed flag on this one since I landed it.
------- Comment #30 From 2008-06-17 22:36:06 PST -------
(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.