Bug 17256 - eliminate default ref. count of 0 in RefCounted class
Summary: eliminate default ref. count of 0 in RefCounted class
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 06:46 PST by Darin Adler
Modified: 2008-02-14 09:31 PST (History)
1 user (show)

See Also:


Attachments
add explicit initialization of RefCounted to 0 almost everywhere (86.95 KB, patch)
2008-02-09 06:49 PST, Darin Adler
no flags Details | Formatted Diff | Diff
patch for next step -- actually remove the default (2.15 KB, patch)
2008-02-10 11:33 PST, Darin Adler
eric: review+
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 06:46:01 PST
To speed up creation of RefCounted objects, we want to change it so objects start with a ref count of 1 rather than starting with a ref count of 0 and then incrementing it to 1.

To do this we need to change RefCounted clients, one at a time, to ask for a count of 1.
Comment 1 Darin Adler 2008-02-09 06:49:52 PST
Created attachment 19016 [details]
add explicit initialization of RefCounted to 0 almost everywhere
Comment 2 Eric Seidel (no email) 2008-02-10 10:55:01 PST
Comment on attachment 19016 [details]
add explicit initialization of RefCounted to 0 almost everywhere

r=me.

It seems like changing everything to start with a ref count of 1 is a lot of work for little gain.  The malloc() should always dwarf the ref() call I would think.  Then again, maybe the parser could benefit from this?  How does one change RefPtr to take advantage of such a change?  (Since a stated goal has long been to get rid of most raw pointers in WebCore and instead us scoped/smart pointers like RefPtr.)
Comment 3 Darin Adler 2008-02-10 11:03:07 PST
(In reply to comment #2)
> It seems like changing everything to start with a ref count of 1 is a lot of
> work for little gain. The malloc() should always dwarf the ref() call I would
> think.

I once thought this too, but recent measurements seem to indicate otherwise. For example, Geoff Garen got a 1% speedup with <http://trac.webkit.org/projects/webkit/changeset/30042>, which is also something that happens only once per object.

I predict this will be well worth it; we should start seeing speed-ups soon.

This also ends up with a much more elegant usage model. We don't want you to be able to create an object with a ref count of 0. That's just a recipe for leaking. Instead we make objects and put them in PassRefPtr right away.

It'll be less elegant during the transition.
Comment 4 Darin Adler 2008-02-10 11:04:15 PST
Comment on attachment 19016 [details]
add explicit initialization of RefCounted to 0 almost everywhere

Eric reviewed and said r=me, but forgot to set review+.
Comment 5 Darin Adler 2008-02-10 11:06:04 PST
(In reply to comment #2)
> How does one change RefPtr to take advantage of such a change?

You don't have to change RefPtr. You just call adoptRef on the result of new rather than putting it in a RefPtr or PassRefPtr without adoptRef.

Here's an example: <http://trac.webkit.org/projects/webkit/changeset/30109>. The short-term scaffolding is the need to add explicit initialization of RefCounted. Once the default is changed to 1 we can remove those again.
Comment 6 Darin Adler 2008-02-10 11:28:31 PST
Committed revision 30122.

This task won't be completely done until I take out the RefCounted default of 0 and we get rid of the last few clients. Then I'll change the default to 1 for the future.
Comment 7 Darin Adler 2008-02-10 11:28:51 PST
Comment on attachment 19016 [details]
add explicit initialization of RefCounted to 0 almost everywhere

Clearing review flag since this was landed.
Comment 8 Darin Adler 2008-02-10 11:33:06 PST
Created attachment 19043 [details]
patch for next step -- actually remove the default
Comment 9 Eric Seidel (no email) 2008-02-10 11:38:38 PST
Comment on attachment 19043 [details]
patch for next step -- actually remove the default

r=me