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

Issue 5643065: Don't call AddDesc() at static init time in LITE_RUNTIME mode. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 1 month ago by philippe
Modified:
11 years, 11 months ago
Reviewers:
digit, pliard1, Jisi Liu, pliard
CC:
kenton
Base URL:
http://protobuf.googlecode.com/svn/trunk
Visibility:
Public.

Description

Don't call AddDesc() at static init time in LITE_RUNTIME mode. This patch makes the generation of StaticDescriptorInitializer_$filename$ depend on whether LITE_RUNTIME is enabled. This lets us significantly decrease the number of static initializers generated by protoc in LITE_RUNTIME mode (used in Chrome). In LITE_RUNTIME mode, $adddescriptorsname$() is called the first time that default_instance() is called (instead of being called during static init). Not calling $adddescriptorsname$() during static initialization adds the following drawback: $adddescriptorsname$() must be thread-safe. Here is a link to the CL that thakis initially created: http://codereview.chromium.org/8740001/ BUG=351

Patch Set 1 #

Patch Set 2 : Update #

Total comments: 13

Patch Set 3 : Update #

Total comments: 1

Patch Set 4 : Update atomicops (now from V8) #

Patch Set 5 : Clean globals.h #

Patch Set 6 : Rebase against CL adding v8's atomicops #

Patch Set 7 : Rebase against CL improving GoogleOnceInit() #

Patch Set 8 : Move helper function to cc file #

Patch Set 9 : Address Pherl and Kenton comments #

Total comments: 2

Patch Set 10 : Address Pherl's comments #

Patch Set 11 : Fix comment #

Patch Set 12 : Force static init when not supported #

Patch Set 13 : Rename get_default_instance() to internal_default_instance() #

Patch Set 14 : Move default_instance_ to cc file #

Patch Set 15 : Make default_instance_ a class static member and do some cleanup #

Patch Set 16 : Minor update #

Total comments: 8

Patch Set 17 : Address Pherl's comments #

Total comments: 2

Patch Set 18 : Fix StaticInitializersForced() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+247 lines, -62 lines) Patch
M src/google/protobuf/compiler/cpp/cpp_file.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +32 lines, -16 lines 0 comments Download
M src/google/protobuf/compiler/cpp/cpp_helpers.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +22 lines, -0 lines 0 comments Download
M src/google/protobuf/compiler/cpp/cpp_helpers.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +60 lines, -0 lines 0 comments Download
M src/google/protobuf/compiler/cpp/cpp_message.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 8 chunks +77 lines, -17 lines 0 comments Download
M src/google/protobuf/compiler/cpp/cpp_message_field.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +10 lines, -2 lines 0 comments Download
M src/google/protobuf/compiler/plugin.pb.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +6 lines, -5 lines 0 comments Download
M src/google/protobuf/descriptor.pb.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 22 chunks +40 lines, -22 lines 0 comments Download

Messages

Total messages: 39
philippe
12 years, 1 month ago (2012-02-08 23:13:36 UTC) #1
Jisi Liu
https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extension_set.cc File src/google/protobuf/extension_set.cc (right): https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extension_set.cc#newcode97 src/google/protobuf/extension_set.cc:97: MutexLock lock(registry_mutex_); well, so this will have performance regressions ...
12 years, 1 month ago (2012-02-09 05:52:54 UTC) #2
digit
https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extension_set.cc File src/google/protobuf/extension_set.cc (right): https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extension_set.cc#newcode65 src/google/protobuf/extension_set.cc:65: Mutex* registry_mutex_ = NULL; just curious: any reason not ...
12 years, 1 month ago (2012-02-09 15:38:12 UTC) #3
kenton
David, Thanks for trying to fix this, but skimming through this patch, I see it ...
12 years, 1 month ago (2012-02-10 04:50:11 UTC) #4
digit
Hello Kenton, On Fri, Feb 10, 2012 at 5:49 AM, Kenton Varda <kenton@google.com> wrote: > ...
12 years, 1 month ago (2012-02-10 09:41:31 UTC) #5
kenton
Note that I only skimmed as I'm no longer on the protobuf project and don't ...
12 years, 1 month ago (2012-02-10 19:22:03 UTC) #6
philippe
Thanks very much for all your comments! I replaced the atomicops implementation (that I stripped) ...
12 years, 1 month ago (2012-02-10 22:38:14 UTC) #7
philippe
On 2012/02/10 22:38:14, philippe wrote: > Thanks very much for all your comments! > > ...
12 years, 1 month ago (2012-02-11 00:17:20 UTC) #8
philippe
On 2012/02/11 00:17:20, philippe wrote: > On 2012/02/10 22:38:14, philippe wrote: > > Thanks very ...
12 years, 1 month ago (2012-02-28 14:17:42 UTC) #9
Jisi Liu
There still seem to be problematic if we use the macros to control the behavior. ...
12 years, 1 month ago (2012-03-01 02:25:42 UTC) #10
kenton
Yes, please make sure that there is only one version of protoc or this feature ...
12 years, 1 month ago (2012-03-01 02:32:59 UTC) #11
pliard1
On 2012/03/01 02:32:59, kenton wrote: > Yes, please make sure that there is only one ...
12 years, 1 month ago (2012-03-01 09:32:17 UTC) #12
philippe
On 2012/03/01 09:32:17, pliard1 wrote: > On 2012/03/01 02:32:59, kenton wrote: > > Yes, please ...
12 years, 1 month ago (2012-03-01 15:11:53 UTC) #13
Jisi Liu
https://codereview.appspot.com/5643065/diff/27007/src/google/protobuf/extension_set.cc File src/google/protobuf/extension_set.cc (right): https://codereview.appspot.com/5643065/diff/27007/src/google/protobuf/extension_set.cc#newcode153 src/google/protobuf/extension_set.cc:153: if (global_state_->is_thread_safe != is_thread_safe) { Hmm.. So I'm thinking ...
12 years, 1 month ago (2012-03-01 20:01:26 UTC) #14
kenton
FYI This is making me really uncomfortable. I think making extension registrations lazy is dangerous, ...
12 years ago (2012-03-01 20:52:32 UTC) #15
philippe
I see your point Kenton. I wish Chrome didn't use extensions too. Sync seems for ...
12 years ago (2012-03-02 10:52:17 UTC) #16
thakis
On Fri, Mar 2, 2012 at 2:52 AM, <philip.liard@gmail.com> wrote: > I see your point ...
12 years ago (2012-03-02 17:56:32 UTC) #17
kenton
On Fri, Mar 2, 2012 at 2:52 AM, <philip.liard@gmail.com> wrote: > I see your point ...
12 years ago (2012-03-02 19:52:12 UTC) #18
kenton
On Fri, Mar 2, 2012 at 9:56 AM, Nico Weber <thakis@chromium.org> wrote: > For what ...
12 years ago (2012-03-02 19:55:08 UTC) #19
philippe
On 2012/03/02 19:55:08, kenton wrote: > On Fri, Mar 2, 2012 at 9:56 AM, Nico ...
12 years ago (2012-03-06 16:25:54 UTC) #20
kenton
get_default_instance() makes me a bit uncomfortable. Two alternative ideas: 1) Call it internal_default_instance(). This at ...
12 years ago (2012-03-06 20:32:08 UTC) #21
philippe
On 2012/03/06 20:32:08, kenton wrote: > get_default_instance() makes me a bit uncomfortable. Two alternative ideas: ...
12 years ago (2012-03-07 11:04:37 UTC) #22
kenton
On Wed, Mar 7, 2012 at 3:04 AM, <philip.liard@gmail.com> wrote: > I tried the anonymous ...
12 years ago (2012-03-07 18:35:14 UTC) #23
pliard1
On 2012/03/07 18:35:14, kenton wrote: > On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com> ...
12 years ago (2012-03-08 10:42:36 UTC) #24
kenton
Ack, I just noticed that you introduced a bunch of out-of-line calls to default_instance() in ...
12 years ago (2012-03-12 21:00:38 UTC) #25
kenton
A thought: We could trigger default instance initialization in a message object's constructor, and then ...
12 years ago (2012-03-12 21:09:49 UTC) #26
pliard
Good point. I might be missing something here. I think that I might have circular ...
12 years ago (2012-03-13 16:45:20 UTC) #27
kenton
On Tue, Mar 13, 2012 at 9:45 AM, Philippe Liard <pliard@google.com> wrote: > I might ...
12 years ago (2012-03-13 18:17:30 UTC) #28
pliard
I sent a new patch set. The code generated for users who don't care about ...
12 years ago (2012-03-14 13:58:50 UTC) #29
kenton
OK, the approach seems good to me now. Thanks for your patience. Pherl should do ...
12 years ago (2012-03-15 21:34:14 UTC) #30
pliard
Thanks for spending so much time reviewing my CLs! I created a separate CL to ...
12 years ago (2012-03-16 11:05:44 UTC) #31
pliard1
On 2012/03/16 11:05:44, pliard wrote: > Thanks for spending so much time reviewing my CLs! ...
11 years, 11 months ago (2012-04-10 08:19:12 UTC) #32
Jisi Liu
Oops.. There'll be an offsite for the rest of week. I'll try to review the ...
11 years, 11 months ago (2012-04-10 10:25:54 UTC) #33
pliard
No worries :) On Tue, Apr 10, 2012 at 12:25 PM, Pherl Liu <liujisi@google.com> wrote: ...
11 years, 11 months ago (2012-04-10 10:40:39 UTC) #34
Jisi Liu
Can you attach a diff for the generated protobuf-lite messages? Also, maybe add a test ...
11 years, 11 months ago (2012-04-16 07:58:04 UTC) #35
pliard1
Hi Pherl, I addressed your comments. Regarding the diff presenting the impact of this change, ...
11 years, 11 months ago (2012-04-20 14:58:46 UTC) #36
Jisi Liu
LGTM Thanks for the work! One comments about the HasExtensions, otherwise LG http://codereview.appspot.com/5643065/diff/92001/src/google/protobuf/compiler/cpp/cpp_helpers.cc File src/google/protobuf/compiler/cpp/cpp_helpers.cc ...
11 years, 11 months ago (2012-05-04 06:25:59 UTC) #37
philippe
Thanks for the review Pherl! I'm waiting for your final LGTM before I commit this. ...
11 years, 11 months ago (2012-05-04 10:43:26 UTC) #38
Jisi Liu
11 years, 11 months ago (2012-05-04 10:53:58 UTC) #39
LGTM
Sign in to reply to this message.

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