Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1115)

Issue 5687064: Add CallOnce() and simple LazyInstance implementation. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by pliard1
Modified:
11 years, 10 months ago
CC:
thakis, v8-dev_googlegroups.com
Base URL:
http://v8.googlecode.com/svn/trunk
Visibility:
Public.

Description

Add CallOnce() and simple LazyInstance implementation. The next CL that will remove static initializers depends on this change. A few notes: - callback.h comes from Protocol Buffers. - This implementation of LazyInstance does not handle global destructors (i.e. the lazy instances are never deleted). Let me know if this is an issue. - A CL adding a similar implementation of CallOnce() (based on a patch sent by Digit) will be sent for review to the Protobuf team very soon. BUG=1859

Patch Set 1 #

Patch Set 2 : Update #

Patch Set 3 : Add comments and remove heap allocation in lazy_instance.h #

Patch Set 4 : Rename GoogleOnceInit to CallOnce #

Total comments: 24

Patch Set 5 : Improve CallOnce() and LazyInstance #

Total comments: 14

Patch Set 6 : Fix memory barrier issue #

Total comments: 2

Patch Set 7 : Reduce callback.h, update SConscript and lint #

Patch Set 8 : Rebase against bleeding_edge #

Total comments: 1

Patch Set 9 : Remove callback.h #

Unified diffs Side-by-side diffs Delta from patch set Stats (+411 lines, -0 lines) Patch
M src/SConscript View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
A src/lazy-instance.h View 1 2 3 4 5 6 1 chunk +216 lines, -0 lines 0 comments Download
A src/once.h View 1 2 3 4 5 6 7 8 1 chunk +108 lines, -0 lines 0 comments Download
A src/once.cc View 1 2 3 4 5 6 7 8 1 chunk +83 lines, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 13
pliard1
12 years, 1 month ago (2012-02-21 16:06:00 UTC) #1
dvyukov
http://codereview.appspot.com/5687064/diff/4004/src/lazy-instance.h File src/lazy-instance.h (right): http://codereview.appspot.com/5687064/diff/4004/src/lazy-instance.h#newcode95 src/lazy-instance.h:95: inline T* MutableInstance() const { inline's are excessive here ...
12 years, 1 month ago (2012-02-22 14:07:35 UTC) #2
pliard1
Hi Dmitry, Thanks for reviewing this CL! The CallOnce() implementation was significantly simplified thanks to ...
12 years, 1 month ago (2012-02-22 18:17:36 UTC) #3
jbates
FYI, I'm working on an alignment fix for LazyInstance in chromium (http://codereview.chromium.org/9186057/). As is, if ...
12 years, 1 month ago (2012-02-23 02:17:41 UTC) #4
pliard
That is interesting! I will pull your AlignedMemory<> implementation (once it is submitted) in a ...
12 years, 1 month ago (2012-02-23 08:41:39 UTC) #5
dvyukov
Much better now There is a minor issue with memory barriers, LGTM other than that ...
12 years, 1 month ago (2012-02-23 09:06:03 UTC) #6
pliard1
http://codereview.appspot.com/5687064/diff/7008/src/lazy-instance.h File src/lazy-instance.h (right): http://codereview.appspot.com/5687064/diff/7008/src/lazy-instance.h#newcode177 src/lazy-instance.h:177: mutable StorageType storage_; On 2012/02/23 09:06:03, dvyukov wrote: > ...
12 years, 1 month ago (2012-02-23 09:27:21 UTC) #7
fschneider
Please make sure that the base URL of your changes in the future always points ...
12 years, 1 month ago (2012-02-23 12:26:44 UTC) #8
pliard1
http://codereview.appspot.com/5687064/diff/7008/src/once.cc File src/once.cc (right): http://codereview.appspot.com/5687064/diff/7008/src/once.cc#newcode41 src/once.cc:41: STATE_INITIALIZED = 0, On 2012/02/23 12:26:44, fschneider wrote: > ...
12 years, 1 month ago (2012-02-23 13:56:35 UTC) #9
fschneider
[+v8-dev] lgtm. Please create the CL on the chromium code review tool: http://codereview.chromium.org/ https://codereview.appspot.com/5687064/diff/9020/src/callback.h File ...
12 years, 1 month ago (2012-02-24 08:34:53 UTC) #10
dvyukov
On 2012/02/23 12:26:44, fschneider wrote: > Please make sure that the base URL of your ...
12 years, 1 month ago (2012-02-24 08:43:29 UTC) #11
pliard1
On 2012/02/24 08:43:29, dvyukov wrote: > On 2012/02/23 12:26:44, fschneider wrote: > > Please make ...
12 years, 1 month ago (2012-02-24 09:29:32 UTC) #12
pliard
12 years, 1 month ago (2012-02-24 11:59:48 UTC) #13
Here is the link to the same CL on chromium codereview:
http://codereview.chromium.org/9447052/

On Fri, Feb 24, 2012 at 10:29 AM, <pliard@chromium.org> wrote:

> On 2012/02/24 08:43:29, dvyukov wrote:
>
>> On 2012/02/23 12:26:44, fschneider wrote:
>> > Please make sure that the base URL of your changes in the future
>>
> always points
>
>> > to the bleeding_edge branch
>> >
(https://v8.googlecode.com/**svn/branches/bleeding_edge<https://v8.googlecode....)
>> and not to
>>
> trunk.
>
>> >
>> > To me it would seems cleaner to reduce callback.h to the minimum
>>
> required for
>
>> > the purpose of static initialization.
>>
>
>  Well, actually if we limit callbacks to "void (*init_func)(Arg*)"
>>
> (instead of
>
>> "void (*init_func)(Arg)" which is IMVHO quite reasonable, then we can
>>
> remove
>
>> callbacks.h entirely. The implementation will just cast "void
>> (*init_func)(Arg*)" to "void (*init_func)(void*)" and Arg* to void*
>>
> and then
>
>> pass it to non-templated implementation.
>>
>
> I removed callback.h.
>
>
http://codereview.appspot.com/**5687064/<http://codereview.appspot.com/5687064/>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b