|
|
DescriptionDon'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() #MessagesTotal messages: 39
https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... File src/google/protobuf/extension_set.cc (right): https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... src/google/protobuf/extension_set.cc:97: MutexLock lock(registry_mutex_); well, so this will have performance regressions for other use-cases. I vaguely remembered there are many places in the codebase that assume and rely on the static initialized default instance. Introducing this lazy init will potentially have the risks that: 1) the default instance is accessed without initialization. 2) the default instance is accessed without locks thus breaks the thread safety. Those should need well testing to find out. I believe chrome only uses the opensource version of protobuf. Could you please use #define/#ifdef a flag to control this behavior. You could pass the flag in the configuration when building protobuf for chrome, which will reduce the potential risks and preserve the current performance for other users. I'm also worried about the maintainability. It's quite easy to break this no-static semantics in future development. We may want to provide an easy way (macro?) so the new static members can be no-static-init friendly (reading the flag). And is it possible to have some tests to detect those? I actually prefer that we do this change in google3, so it also reduces the risk when we up-integrate this change back. But if we protect it using #ifdefs, I guess it's fine.
Sign in to reply to this message.
https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... File src/google/protobuf/extension_set.cc (right): https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... src/google/protobuf/extension_set.cc:65: Mutex* registry_mutex_ = NULL; just curious: any reason not to declare this as static? Same question for the registry_ global variable above, and registry_init_ below. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... src/google/protobuf/extension_set.cc:97: MutexLock lock(registry_mutex_); I would say that a feature based macro is a good idea at the moment. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... File src/google/protobuf/stubs/once.h (left): https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... src/google/protobuf/stubs/once.h:101: // Note: Double-checked locking is safe on x86. Quick note for upstream authors: double-checked locking is not safe on ARM, and Windows 8 will likely run on this platform real soon now. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... File src/google/protobuf/stubs/once.h (right): https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... src/google/protobuf/stubs/once.h:112: // passing therefore we use our own implementation based on a recursive mutex. For the record, locking/unlocking a recursive mutex can be a lot slower than using a pthread_once_t. I have benchmarked than on x86_64, it is 3x slower (1.75ns vs 0.67ns), on Android ARM, it is 6x slower with Bionic (110ns vs 18ns) or 16x with GLibc (192ns vs 12ns). See comment below about this. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... src/google/protobuf/stubs/once.h:119: typedef MutexBoolPair ProtobufOnceType; Is there a reason to define the MutexBoolPair struct type? I.e. why don't we just put this into struct ProtobufOnceType instead. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... src/google/protobuf/stubs/once.h:148: internal::FunctionClosure0 func(init_func, false); see comment below. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... src/google/protobuf/stubs/once.h:156: GoogleOnceInitImpl(once, &func); locking/unlocking the mutex on each call is going to be very costly. It would be better to use atomic operations to ensure that our common case remains fast, i.e. something that looks like (when expanded): if (atomic_load_acquire(&once->already_done) != true) { mutex_lock(...) if (once->already_done != true) { closure->Run(); // relies on mutex_unlock's implicit release barrier once->already_done = true; } mutex_unlock(...) } This requires changing the type of 'already_done' to an atomic type (e.g. Atomic32 or AtomicWord). The condensed version (for the inlined template function) would be something like: if (atomic_load_acquire(&once->already_done) != true) { GoogleOnceInitImpl(once, &func); } This would ensure that the only cost, in the common case, if the atomic load + acquire semantics, i.e. equivalent to a pthread_once() call.
Sign in to reply to this message.
David, Thanks for trying to fix this, but skimming through this patch, I see it creating a lot of potential problems. Protocol Buffers is library code, and as such we have more stringent requirements for interface cleanliness, portability, and robustness. Some things I'm seeing that look problematic: - The protobuf library is used on compilers other than just MSVC and GCC, and on platforms other than Windows, Mac, and Linux. We either need a * really* portable atomic ops implementation, or we need a fallback for other compilers and platforms. - Your build_defs.h defines lots of macros in a header that will end up being included by client code. Clients will yell at us if you do this. You need to prefix those macros with PROTOBUF_ or something. Many of the macros appear to be irrelevant to protobufs anyawy. - Some users rely on extensions being registered in the global extension registry without them actually touching the translation unit defining them. I think your change will break them. On Thu, Feb 9, 2012 at 7:38 AM, <digit@google.com> wrote: > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/extension_set.**cc<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<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 to declare this as static? > Same question for the registry_ global variable above, and > registry_init_ below. > > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/extension_set.**cc#newcode97<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_); > I would say that a feature based macro is a good idea at the moment. > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/stubs/once.h<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h> > File src/google/protobuf/stubs/**once.h (left): > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/stubs/once.h#**oldcode101<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#oldcode101> > src/google/protobuf/stubs/**once.h:101: // Note: Double-checked locking > is safe on x86. > Quick note for upstream authors: double-checked locking is not safe on > ARM, and Windows 8 will likely run on this platform real soon now. > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/stubs/once.h<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h> > File src/google/protobuf/stubs/**once.h (right): > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/stubs/once.h#**newcode112<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#newcode112> > src/google/protobuf/stubs/**once.h:112: // passing therefore we use our > own implementation based on a recursive mutex. > For the record, locking/unlocking a recursive mutex can be a lot slower > than using a pthread_once_t. I have benchmarked than on x86_64, it is 3x > slower (1.75ns vs 0.67ns), on Android ARM, it is 6x slower with Bionic > (110ns vs 18ns) or 16x with GLibc (192ns vs 12ns). > > See comment below about this. > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/stubs/once.h#**newcode119<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#newcode119> > src/google/protobuf/stubs/**once.h:119: typedef MutexBoolPair > ProtobufOnceType; > Is there a reason to define the MutexBoolPair struct type? I.e. why > don't we just put this into struct ProtobufOnceType instead. > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/stubs/once.h#**newcode148<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#newcode148> > src/google/protobuf/stubs/**once.h:148: internal::FunctionClosure0 > func(init_func, false); > see comment below. > > https://codereview.appspot.**com/5643065/diff/1011/src/** > google/protobuf/stubs/once.h#**newcode156<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#newcode156> > src/google/protobuf/stubs/**once.h:156: GoogleOnceInitImpl(once, &func); > locking/unlocking the mutex on each call is going to be very costly. It > would be better to use atomic operations to ensure that our common case > remains fast, i.e. something that looks like (when expanded): > > if (atomic_load_acquire(&once->**already_done) != true) { > mutex_lock(...) > if (once->already_done != true) { > closure->Run(); > // relies on mutex_unlock's implicit release barrier > once->already_done = true; > } > mutex_unlock(...) > } > > This requires changing the type of 'already_done' to an atomic type > (e.g. Atomic32 or AtomicWord). > > The condensed version (for the inlined template function) would be > something like: > > if (atomic_load_acquire(&once->**already_done) != true) { > GoogleOnceInitImpl(once, &func); > } > > This would ensure that the only cost, in the common case, if the atomic > load + acquire semantics, i.e. equivalent to a pthread_once() call. > > https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >
Sign in to reply to this message.
Hello Kenton, On Fri, Feb 10, 2012 at 5:49 AM, Kenton Varda <kenton@google.com> wrote: > David, > > Thanks for trying to fix this, but skimming through this patch, I see it > creating a lot of potential problems. Protocol Buffers is library code, > and as such we have more stringent requirements for interface cleanliness, > portability, and robustness. > Thanks for reviewing this. We understand the problem and I actually encouraged Philippe to send a first patch for review as soon as possible to have your input on it. We know it's not ready yet but we wanted your feedback on it (thanks again). To add a bit of context, we're trying to remove all static initializers from Chrome on Android, because profiling has shown that they were a real drag to startup time in the case of a cold start, mainly due to the page faults they generate both in the .text and .data sections. This patch could remove 47 static initializers in a single stroke for our project. Some things I'm seeing that look problematic: > > - The protobuf library is used on compilers other than just MSVC and GCC, > and on platforms other than Windows, Mac, and Linux. We either need a * > really* portable atomic ops implementation, or we need a fallback for > other compilers and platforms. > > I think a good idea would be to have a configuration macro like PROTOBUF_NO_STATIC_INITIALIZERS that could be defined only when the feature is desired and the code is built on a supported platform/compiler combination. For atomics, I'd suggest borrowing the v8 ones (i.e. the atomicops* files on this web page<http://code.google.com/p/v8/source/browse#svn%2Ftrunk%2Fsrc>), which are a slightly simplified version of the Chromium ones (e.g. they don't need the macros defined in build_config.h) and cover a lot more combos. They are also made entirely of inlined functions provided by header files, which means that they won't add any bloat to the library if they are not used, i.e. if PROTOBUF_NO_STATIC_INITIALIZERS is not defined. > - Your build_defs.h defines lots of macros in a header that will end up > being included by client code. Clients will yell at us if you do this. > You need to prefix those macros with PROTOBUF_ or something. Many of the > macros appear to be irrelevant to protobufs anyawy. > > I think the build_config.h can be removed if we provide a better atomicops headers. > - Some users rely on extensions being registered in the global extension > registry without them actually touching the translation unit defining them. > I think your change will break them. > > On Thu, Feb 9, 2012 at 7:38 AM, <digit@google.com> wrote: > >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/extension_set.**cc<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<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 to declare this as static? >> Same question for the registry_ global variable above, and >> registry_init_ below. >> >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/extension_set.**cc#newcode97<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_); >> I would say that a feature based macro is a good idea at the moment. >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/stubs/once.h<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h> >> File src/google/protobuf/stubs/**once.h (left): >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/stubs/once.h#**oldcode101<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#oldcode101> >> src/google/protobuf/stubs/**once.h:101: // Note: Double-checked locking >> is safe on x86. >> Quick note for upstream authors: double-checked locking is not safe on >> ARM, and Windows 8 will likely run on this platform real soon now. >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/stubs/once.h<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h> >> File src/google/protobuf/stubs/**once.h (right): >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/stubs/once.h#**newcode112<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#newcode112> >> src/google/protobuf/stubs/**once.h:112: // passing therefore we use our >> own implementation based on a recursive mutex. >> For the record, locking/unlocking a recursive mutex can be a lot slower >> than using a pthread_once_t. I have benchmarked than on x86_64, it is 3x >> slower (1.75ns vs 0.67ns), on Android ARM, it is 6x slower with Bionic >> (110ns vs 18ns) or 16x with GLibc (192ns vs 12ns). >> >> See comment below about this. >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/stubs/once.h#**newcode119<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#newcode119> >> src/google/protobuf/stubs/**once.h:119: typedef MutexBoolPair >> ProtobufOnceType; >> Is there a reason to define the MutexBoolPair struct type? I.e. why >> don't we just put this into struct ProtobufOnceType instead. >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/stubs/once.h#**newcode148<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#newcode148> >> src/google/protobuf/stubs/**once.h:148: internal::FunctionClosure0 >> func(init_func, false); >> see comment below. >> >> https://codereview.appspot.**com/5643065/diff/1011/src/** >> google/protobuf/stubs/once.h#**newcode156<https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/once.h#newcode156> >> src/google/protobuf/stubs/**once.h:156: GoogleOnceInitImpl(once, &func); >> locking/unlocking the mutex on each call is going to be very costly. It >> would be better to use atomic operations to ensure that our common case >> remains fast, i.e. something that looks like (when expanded): >> >> if (atomic_load_acquire(&once->**already_done) != true) { >> mutex_lock(...) >> if (once->already_done != true) { >> closure->Run(); >> // relies on mutex_unlock's implicit release barrier >> once->already_done = true; >> } >> mutex_unlock(...) >> } >> >> This requires changing the type of 'already_done' to an atomic type >> (e.g. Atomic32 or AtomicWord). >> >> The condensed version (for the inlined template function) would be >> something like: >> >> if (atomic_load_acquire(&once->**already_done) != true) { >> GoogleOnceInitImpl(once, &func); >> } >> >> This would ensure that the only cost, in the common case, if the atomic >> load + acquire semantics, i.e. equivalent to a pthread_once() call. >> >> https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >> > >
Sign in to reply to this message.
Note that I only skimmed as I'm no longer on the protobuf project and don't have the cycles to do a full review. (Also, I was CC'd late and missed the fact that philippe is the author.) On Fri, Feb 10, 2012 at 1:41 AM, David Turner <digit@google.com> wrote: > I think a good idea would be to have a configuration macro like > PROTOBUF_NO_STATIC_INITIALIZERS that could be defined only when the feature > is desired and the code is built on a supported platform/compiler > combination. For atomics, I'd suggest borrowing the v8 ones (i.e. the > atomicops* files on this web page<http://code.google.com/p/v8/source/browse#svn%2Ftrunk%2Fsrc>), > which are a slightly simplified version of the Chromium ones (e.g. they > don't need the macros defined in build_config.h) and cover a lot more > combos. They are also made entirely of inlined functions provided by header > files, which means that they won't add any bloat to the library if they are > not used, i.e. if PROTOBUF_NO_STATIC_INITIALIZERS is not defined. > We actually have a need for atomic ops for other reasons -- things we've been putting off for some time due to the portability issues. You spotted one of them: the terrible way that we implement "once" operations on Win32. It would be nice to replace that, and pthread_once, with something coded directly in terms of atomics. There is probably no way around sacrificing some portability here, but we should sacrifice as little as possible. It sounds like getting the v8 atomic ops into the protobuf library would be a great first step. Perhaps this should be done in multiple patches.
Sign in to reply to this message.
Thanks very much for all your comments! I replaced the atomicops implementation (that I stripped) initially coming from Chromium with the full one from V8. It implements the common atomic operations and supports an additional architecture (MIPS). I also prefixed all the #defines (still required for most of them) with GOOGLE_PROTOBUF_. Now the code generated by protoc changes only if a flag is enabled (-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZERS). That only works in LITE_RUNTIME mode (this is checked in cpp_generator.cc). The flag also applies to extension_set.cc (there is no mutex anymore if the flag is not enabled). Therefore this change should not affect too much the clients that do not enable the flag (assuming that V8's atomicops implementation covers all the platforms currently supported by Protobuf). I am also testing this change currently on Windows (I am having a few build issues with GTest for now). Regarding GoogleOnceInit on Windows, I am experimenting the one-time init API (supporting compile-time initialization (no need for static initializer)) provided by Windows Vista/Server 2003 and more recent. This seems to be a nice replacement for the current implementation (based on Windows CRITICAL_SECTION). However Windows XP does not seem to provide such capabilities. We may have to implement something manually based on atomic operations in that case. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... File src/google/protobuf/extension_set.cc (right): https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... src/google/protobuf/extension_set.cc:65: Mutex* registry_mutex_ = NULL; On 2012/02/09 15:38:13, digit wrote: > just curious: any reason not to declare this as static? > Same question for the registry_ global variable above, and registry_init_ below. It is not obvious in the diff, but this part of the file is part of an anonymous namespace. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... src/google/protobuf/extension_set.cc:97: MutexLock lock(registry_mutex_); On 2012/02/09 15:38:13, digit wrote: > I would say that a feature based macro is a good idea at the moment. Indeed. I made the compilation of this code conditional with a define. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... File src/google/protobuf/stubs/once.h (right): https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... src/google/protobuf/stubs/once.h:119: typedef MutexBoolPair ProtobufOnceType; On 2012/02/09 15:38:13, digit wrote: > Is there a reason to define the MutexBoolPair struct type? I.e. why don't we > just put this into struct ProtobufOnceType instead. My only reason for now is that I want to avoid touching too much the WIN32 code. In the next CL, I will address the WIN32 issue. We will have indeed a single struct (without any method probably) shared by both implementation. I am currently having some build issues with GTest on Visual Studio 2010. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... src/google/protobuf/stubs/once.h:148: internal::FunctionClosure0 func(init_func, false); On 2012/02/09 15:38:13, digit wrote: > see comment below. Done. https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... src/google/protobuf/stubs/once.h:156: GoogleOnceInitImpl(once, &func); On 2012/02/09 15:38:13, digit wrote: > locking/unlocking the mutex on each call is going to be very costly. It would be > better to use atomic operations to ensure that our common case remains fast, > i.e. something that looks like (when expanded): > > if (atomic_load_acquire(&once->already_done) != true) { > mutex_lock(...) > if (once->already_done != true) { > closure->Run(); > // relies on mutex_unlock's implicit release barrier > once->already_done = true; > } > mutex_unlock(...) > } > > This requires changing the type of 'already_done' to an atomic type (e.g. > Atomic32 or AtomicWord). > > The condensed version (for the inlined template function) would be something > like: > > if (atomic_load_acquire(&once->already_done) != true) { > GoogleOnceInitImpl(once, &func); > } > > This would ensure that the only cost, in the common case, if the atomic load + > acquire semantics, i.e. equivalent to a pthread_once() call. Thanks for all the details :) https://codereview.appspot.com/5643065/diff/8001/src/google/protobuf/stubs/bu... File src/google/protobuf/stubs/build_config.h (right): https://codereview.appspot.com/5643065/diff/8001/src/google/protobuf/stubs/bu... src/google/protobuf/stubs/build_config.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. It seems that there is also a file similar to this one in V8 (globals.h) used by the atomicops implementation. But it is safer as all the flags are prefixed by "V8_".
Sign in to reply to this message.
On 2012/02/10 22:38:14, philippe wrote: > Thanks very much for all your comments! > > I replaced the atomicops implementation (that I stripped) initially coming from > Chromium with the full one from V8. It implements the common atomic operations > and supports an additional architecture (MIPS). I also prefixed all the #defines > (still required for most of them) with GOOGLE_PROTOBUF_. > > Now the code generated by protoc changes only if a flag is enabled > (-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZERS). That only works in LITE_RUNTIME mode > (this is checked in cpp_generator.cc). > The flag also applies to extension_set.cc (there is no mutex anymore if the flag > is not enabled). > Therefore this change should not affect too much the clients that do not enable > the flag (assuming that V8's atomicops implementation covers all the platforms > currently supported by Protobuf). > > I am also testing this change currently on Windows (I am having a few build > issues with GTest for now). > Regarding GoogleOnceInit on Windows, I am experimenting the one-time init API > (supporting compile-time initialization (no need for static initializer)) > provided by Windows Vista/Server 2003 and more recent. This seems to be a nice > replacement for the current implementation (based on Windows CRITICAL_SECTION). > However Windows XP does not seem to provide such capabilities. We may have to > implement something manually based on atomic operations in that case. > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... > File src/google/protobuf/extension_set.cc (right): > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... > src/google/protobuf/extension_set.cc:65: Mutex* registry_mutex_ = NULL; > On 2012/02/09 15:38:13, digit wrote: > > just curious: any reason not to declare this as static? > > Same question for the registry_ global variable above, and registry_init_ > below. > > It is not obvious in the diff, but this part of the file is part of an anonymous > namespace. > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... > src/google/protobuf/extension_set.cc:97: MutexLock lock(registry_mutex_); > On 2012/02/09 15:38:13, digit wrote: > > I would say that a feature based macro is a good idea at the moment. > > Indeed. I made the compilation of this code conditional with a define. > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... > File src/google/protobuf/stubs/once.h (right): > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... > src/google/protobuf/stubs/once.h:119: typedef MutexBoolPair ProtobufOnceType; > On 2012/02/09 15:38:13, digit wrote: > > Is there a reason to define the MutexBoolPair struct type? I.e. why don't we > > just put this into struct ProtobufOnceType instead. > > My only reason for now is that I want to avoid touching too much the WIN32 code. > > In the next CL, I will address the WIN32 issue. We will have indeed a single > struct (without any method probably) shared by both implementation. > > I am currently having some build issues with GTest on Visual Studio 2010. > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... > src/google/protobuf/stubs/once.h:148: internal::FunctionClosure0 func(init_func, > false); > On 2012/02/09 15:38:13, digit wrote: > > see comment below. > > Done. > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... > src/google/protobuf/stubs/once.h:156: GoogleOnceInitImpl(once, &func); > On 2012/02/09 15:38:13, digit wrote: > > locking/unlocking the mutex on each call is going to be very costly. It would > be > > better to use atomic operations to ensure that our common case remains fast, > > i.e. something that looks like (when expanded): > > > > if (atomic_load_acquire(&once->already_done) != true) { > > mutex_lock(...) > > if (once->already_done != true) { > > closure->Run(); > > // relies on mutex_unlock's implicit release barrier > > once->already_done = true; > > } > > mutex_unlock(...) > > } > > > > This requires changing the type of 'already_done' to an atomic type (e.g. > > Atomic32 or AtomicWord). > > > > The condensed version (for the inlined template function) would be something > > like: > > > > if (atomic_load_acquire(&once->already_done) != true) { > > GoogleOnceInitImpl(once, &func); > > } > > > > This would ensure that the only cost, in the common case, if the atomic load + > > acquire semantics, i.e. equivalent to a pthread_once() call. > > Thanks for all the details :) > > https://codereview.appspot.com/5643065/diff/8001/src/google/protobuf/stubs/bu... > File src/google/protobuf/stubs/build_config.h (right): > > https://codereview.appspot.com/5643065/diff/8001/src/google/protobuf/stubs/bu... > src/google/protobuf/stubs/build_config.h:1: // Copyright (c) 2012 The Chromium > Authors. All rights reserved. > It seems that there is also a file similar to this one in V8 (globals.h) used by > the atomicops implementation. But it is safer as all the flags are prefixed by > "V8_". As discussed with David, I am moving the v8's atomicops addition to a separate CL: https://codereview.appspot.com/5654057/
Sign in to reply to this message.
On 2012/02/11 00:17:20, philippe wrote: > On 2012/02/10 22:38:14, philippe wrote: > > Thanks very much for all your comments! > > > > I replaced the atomicops implementation (that I stripped) initially coming > from > > Chromium with the full one from V8. It implements the common atomic operations > > and supports an additional architecture (MIPS). I also prefixed all the > #defines > > (still required for most of them) with GOOGLE_PROTOBUF_. > > > > Now the code generated by protoc changes only if a flag is enabled > > (-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZERS). That only works in LITE_RUNTIME > mode > > (this is checked in cpp_generator.cc). > > The flag also applies to extension_set.cc (there is no mutex anymore if the > flag > > is not enabled). > > Therefore this change should not affect too much the clients that do not > enable > > the flag (assuming that V8's atomicops implementation covers all the platforms > > currently supported by Protobuf). > > > > I am also testing this change currently on Windows (I am having a few build > > issues with GTest for now). > > Regarding GoogleOnceInit on Windows, I am experimenting the one-time init API > > (supporting compile-time initialization (no need for static initializer)) > > provided by Windows Vista/Server 2003 and more recent. This seems to be a nice > > replacement for the current implementation (based on Windows > CRITICAL_SECTION). > > However Windows XP does not seem to provide such capabilities. We may have to > > implement something manually based on atomic operations in that case. > > > > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... > > File src/google/protobuf/extension_set.cc (right): > > > > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... > > src/google/protobuf/extension_set.cc:65: Mutex* registry_mutex_ = NULL; > > On 2012/02/09 15:38:13, digit wrote: > > > just curious: any reason not to declare this as static? > > > Same question for the registry_ global variable above, and registry_init_ > > below. > > > > It is not obvious in the diff, but this part of the file is part of an > anonymous > > namespace. > > > > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/extensio... > > src/google/protobuf/extension_set.cc:97: MutexLock lock(registry_mutex_); > > On 2012/02/09 15:38:13, digit wrote: > > > I would say that a feature based macro is a good idea at the moment. > > > > Indeed. I made the compilation of this code conditional with a define. > > > > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... > > File src/google/protobuf/stubs/once.h (right): > > > > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... > > src/google/protobuf/stubs/once.h:119: typedef MutexBoolPair ProtobufOnceType; > > On 2012/02/09 15:38:13, digit wrote: > > > Is there a reason to define the MutexBoolPair struct type? I.e. why don't we > > > just put this into struct ProtobufOnceType instead. > > > > My only reason for now is that I want to avoid touching too much the WIN32 > code. > > > > In the next CL, I will address the WIN32 issue. We will have indeed a single > > struct (without any method probably) shared by both implementation. > > > > I am currently having some build issues with GTest on Visual Studio 2010. > > > > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... > > src/google/protobuf/stubs/once.h:148: internal::FunctionClosure0 > func(init_func, > > false); > > On 2012/02/09 15:38:13, digit wrote: > > > see comment below. > > > > Done. > > > > > https://codereview.appspot.com/5643065/diff/1011/src/google/protobuf/stubs/on... > > src/google/protobuf/stubs/once.h:156: GoogleOnceInitImpl(once, &func); > > On 2012/02/09 15:38:13, digit wrote: > > > locking/unlocking the mutex on each call is going to be very costly. It > would > > be > > > better to use atomic operations to ensure that our common case remains fast, > > > i.e. something that looks like (when expanded): > > > > > > if (atomic_load_acquire(&once->already_done) != true) { > > > mutex_lock(...) > > > if (once->already_done != true) { > > > closure->Run(); > > > // relies on mutex_unlock's implicit release barrier > > > once->already_done = true; > > > } > > > mutex_unlock(...) > > > } > > > > > > This requires changing the type of 'already_done' to an atomic type (e.g. > > > Atomic32 or AtomicWord). > > > > > > The condensed version (for the inlined template function) would be something > > > like: > > > > > > if (atomic_load_acquire(&once->already_done) != true) { > > > GoogleOnceInitImpl(once, &func); > > > } > > > > > > This would ensure that the only cost, in the common case, if the atomic load > + > > > acquire semantics, i.e. equivalent to a pthread_once() call. > > > > Thanks for all the details :) > > > > > https://codereview.appspot.com/5643065/diff/8001/src/google/protobuf/stubs/bu... > > File src/google/protobuf/stubs/build_config.h (right): > > > > > https://codereview.appspot.com/5643065/diff/8001/src/google/protobuf/stubs/bu... > > src/google/protobuf/stubs/build_config.h:1: // Copyright (c) 2012 The Chromium > > Authors. All rights reserved. > > It seems that there is also a file similar to this one in V8 (globals.h) used > by > > the atomicops implementation. But it is safer as all the flags are prefixed by > > "V8_". > > As discussed with David, I am moving the v8's atomicops addition to a separate > CL: https://codereview.appspot.com/5654057/ This CL is still alive :) It depends on the one improving GoogleOnceInit() (that I just sent for review): http://codereview.appspot.com/5708043/
Sign in to reply to this message.
There still seem to be problematic if we use the macros to control the behavior. Let's take a step back. There are 3 parts: 1) runtime library, i.e. the libprotobuf, (including extension_set.cc) 2) the protoc compiler. 3) the protoc generated code. We use version macros to ensure that certain version of protoc generated code only works with certain version of libprotobuf. However, when this macro is introduced, the protoc and libprotobuf both have 2 variants even if the version number matches. 1) we will have to make sure that the macro defined in protoc matches the one in libprotobuf. 2) what if the client needs to run 2 different dynamic linked binary, but they ask for different static init behavior? My gut feeling is protoc should be able to generate both variants, controlled by .proto option and/or command line flags. libprotobuf should also be able to work with both variants of generated files by providing different APIs for generated message.
Sign in to reply to this message.
Yes, please make sure that there is only one version of protoc or this feature will be very painful to use. E.g. if someone installs protoc via the ubuntu package, how would they use this feature? I think protoc should simply generate code that itself contains #ifdefs for GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER. There's no need for an option or multiple versions of protoc at all, then. On Wed, Feb 29, 2012 at 6:25 PM, <liujisi@google.com> wrote: > There still seem to be problematic if we use the macros to control the > behavior. Let's take a step back. There are 3 parts: > > 1) runtime library, i.e. the libprotobuf, (including extension_set.cc) > 2) the protoc compiler. > 3) the protoc generated code. > > We use version macros to ensure that certain version of protoc generated > code only works with certain version of libprotobuf. > > However, when this macro is introduced, the protoc and libprotobuf both > have 2 variants even if the version number matches. > 1) we will have to make sure that the macro defined in protoc matches > the one in libprotobuf. > 2) what if the client needs to run 2 different dynamic linked binary, > but they ask for different static init behavior? > > My gut feeling is protoc should be able to generate both variants, > controlled by .proto option and/or command line flags. libprotobuf > should also be able to work with both variants of generated files by > providing different APIs for generated message. > > > https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >
Sign in to reply to this message.
On 2012/03/01 02:32:59, kenton wrote: > Yes, please make sure that there is only one version of protoc or this > feature will be very painful to use. E.g. if someone installs protoc via > the ubuntu package, how would they use this feature? > > I think protoc should simply generate code that itself contains #ifdefs > for GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER. There's no need for an option > or multiple versions of protoc at all, then. > > On Wed, Feb 29, 2012 at 6:25 PM, <mailto:liujisi@google.com> wrote: > > > There still seem to be problematic if we use the macros to control the > > behavior. Let's take a step back. There are 3 parts: > > > > 1) runtime library, i.e. the libprotobuf, (including extension_set.cc) > > 2) the protoc compiler. > > 3) the protoc generated code. > > > > We use version macros to ensure that certain version of protoc generated > > code only works with certain version of libprotobuf. > > > > However, when this macro is introduced, the protoc and libprotobuf both > > have 2 variants even if the version number matches. > > 1) we will have to make sure that the macro defined in protoc matches > > the one in libprotobuf. > > 2) what if the client needs to run 2 different dynamic linked binary, > > but they ask for different static init behavior? > > > > My gut feeling is protoc should be able to generate both variants, > > controlled by .proto option and/or command line flags. libprotobuf > > should also be able to work with both variants of generated files by > > providing different APIs for generated message. > > > > > > > https://codereview.appspot.**com/5643065/%3Chttps://codereview.appspot.com/56...> > > I agree with your points. I can totally make the changes to have a single version of protoc. It would add #ifdefs in the generated code instead. I am a bit more concerned about the runtime library. Currently the changes are limited to extension_set.cc (mutex). extension_set.h will also be modified by the next CL making ExtensionIdentifier POD. You said in your first message that we can't always afford the cost of the mutex especially if it is not needed (which I understand). Naively, I could pass a boolean to Register() to make the use of the mutex conditional. But what if some clients build their .pb.cc files with different flags (some files with the no static init. flag and some others without). We would have clients accessing the (global) extension registry without using the mutex (the ones not using the no static init. flag) while some others (using the flag) could call Register() at the same time. Can we assume that clients use the same flags to build all their .pb.cc files (for a single binary)? In my opinion, I don't see how mixing static and no static behaviors in a single binary could be useful. In that case, I think that the best we can do is detecting at runtime that Register() is called with different boolean parameters and generate an error. I will start making the changes.
Sign in to reply to this message.
On 2012/03/01 09:32:17, pliard1 wrote: > On 2012/03/01 02:32:59, kenton wrote: > > Yes, please make sure that there is only one version of protoc or this > > feature will be very painful to use. E.g. if someone installs protoc via > > the ubuntu package, how would they use this feature? > > > > I think protoc should simply generate code that itself contains #ifdefs > > for GOOGLE_PROTOBUF_NO_STATIC_INITIALIZER. There's no need for an option > > or multiple versions of protoc at all, then. > > > > On Wed, Feb 29, 2012 at 6:25 PM, <mailto:liujisi@google.com> wrote: > > > > > There still seem to be problematic if we use the macros to control the > > > behavior. Let's take a step back. There are 3 parts: > > > > > > 1) runtime library, i.e. the libprotobuf, (including extension_set.cc) > > > 2) the protoc compiler. > > > 3) the protoc generated code. > > > > > > We use version macros to ensure that certain version of protoc generated > > > code only works with certain version of libprotobuf. > > > > > > However, when this macro is introduced, the protoc and libprotobuf both > > > have 2 variants even if the version number matches. > > > 1) we will have to make sure that the macro defined in protoc matches > > > the one in libprotobuf. > > > 2) what if the client needs to run 2 different dynamic linked binary, > > > but they ask for different static init behavior? > > > > > > My gut feeling is protoc should be able to generate both variants, > > > controlled by .proto option and/or command line flags. libprotobuf > > > should also be able to work with both variants of generated files by > > > providing different APIs for generated message. > > > > > > > > > > > > https://codereview.appspot.**com/5643065/%253Chttps://codereview.appspot.com/...> > > > > > I agree with your points. > > I can totally make the changes to have a single version of protoc. It would add > #ifdefs in the generated code instead. > I am a bit more concerned about the runtime library. Currently the changes are > limited to extension_set.cc (mutex). extension_set.h will also be modified by > the next CL making ExtensionIdentifier POD. > You said in your first message that we can't always afford the cost of the mutex > especially if it is not needed (which I understand). > Naively, I could pass a boolean to Register() to make the use of the mutex > conditional. But what if some clients build their .pb.cc files with different > flags (some files with the no static init. flag and some others without). We > would have clients accessing the (global) extension registry without using the > mutex (the ones not using the no static init. flag) while some others (using the > flag) could call Register() at the same time. > Can we assume that clients use the same flags to build all their .pb.cc files > (for a single binary)? In my opinion, I don't see how mixing static and no > static behaviors in a single binary could be useful. > In that case, I think that the best we can do is detecting at runtime that > Register() is called with different boolean parameters and generate an error. > > I will start making the changes. I made the change. It obviously adds a few #ifdefs in the generated code but we know have a single version (i.e. no additional compile flag) of both protoc and the runtime library. I had to make a few more changes in extension_set.cc/message_lite.{h,cc} to make the use of the mutex conditional at runtime.
Sign in to reply to this message.
https://codereview.appspot.com/5643065/diff/27007/src/google/protobuf/extensi... File src/google/protobuf/extension_set.cc (right): https://codereview.appspot.com/5643065/diff/27007/src/google/protobuf/extensi... src/google/protobuf/extension_set.cc:153: if (global_state_->is_thread_safe != is_thread_safe) { Hmm.. So I'm thinking about the pre-built descriptor.pb.cc, which I believe doesn't define extensions currently. It looks like we will not be able to define extension in pre-built protos in the future. This becomes a more general problem that non-static-init users cannot depend on other libraries that also use protobuf (normally with static init). How about we keep 2 ExtensionRegistry? I guess the lock-free one should take precedence?
Sign in to reply to this message.
FYI This is making me really uncomfortable. I think making extension registrations lazy is dangerous, because it will be very hard for users to detect and understand bugs caused by extensions not being registered yet at the time they are parsed. This is the kind of thing that is basically impossible to test in unit tests, because behavior may depend on the order in which completely unrelated pieces of code are executed. I would prefer to follow one of the following approaches: 1) Continue to register extensions eagerly. Chrome should simply avoid using extensions if it doesn't like this. Since Chrome is a single binary that always links in everything, it seems like it doesn't have much to gain from extensions anyway. 2) Do not register extensions automatically at all. Users should build an extension registry explicitly, and then pass it in when parsing messages. This is the approach used in Java, and I wish I had used this approach in C++ from the beginning, because singleton registries are problematic for a lot more reasons than just the static initializers they required. We would need to define an ExtensionRegistry interface, and probably make it possible to attach an ExtensionRegistry to a CodedInputStream while parsing (CodedInputStream actually already has a SetExtensionRegistry() method that does something similar, but it's based on descriptors, which are not used in lite mode). I think #2 would be a pretty nice change to have in general. We should probably make it possible to enable this approach via a .proto-level option rather than a #define, since it's a significant behavioral change that people may want to be able to use on a per-file basis. On Thu, Mar 1, 2012 at 12:01 PM, <liujisi@google.com> wrote: > > https://codereview.appspot.**com/5643065/diff/27007/src/** > google/protobuf/extension_set.**cc<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<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 about the pre-built descriptor.pb.cc, which I > believe doesn't define extensions currently. It looks like we will not > be able to define extension in pre-built protos in the future. > > This becomes a more general problem that non-static-init users cannot > depend on other libraries that also use protobuf (normally with static > init). > > How about we keep 2 ExtensionRegistry? I guess the lock-free one should > take precedence? > > https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >
Sign in to reply to this message.
I see your point Kenton. I wish Chrome didn't use extensions too. Sync seems for some reason to rely quite heavily on them. I don't think I can easily get rid of them. I initially tried to address the extension registration issue in a separate CL which makes ExtensionIdentifier POD at the same time: http://codereview.appspot.com/5721046/ While I am considering your second approach, I would be grateful if you could briefly take a look at the changes in extension_set.h and the comments in cpp_extension.cc and tell me what you think. I think that this change would guarantee that extensions are always registered before the user queries the extension registry. https://codereview.appspot.com/5643065/diff/27007/src/google/protobuf/extensi... File src/google/protobuf/extension_set.cc (right): https://codereview.appspot.com/5643065/diff/27007/src/google/protobuf/extensi... src/google/protobuf/extension_set.cc:153: if (global_state_->is_thread_safe != is_thread_safe) { On 2012/03/01 20:01:26, Pherl Liu wrote: > Hmm.. So I'm thinking about the pre-built descriptor.pb.cc, which I believe > doesn't define extensions currently. It looks like we will not be able to define > extension in pre-built protos in the future. > > This becomes a more general problem that non-static-init users cannot depend on > other libraries that also use protobuf (normally with static init). > > How about we keep 2 ExtensionRegistry? I guess the lock-free one should take > precedence? Good point. About your last sentence mentioning precedence, I might be missing something. It seems that I can directly use the right extension registry since in all cases containing_type is provided, which tells me which extension registry I have to use.
Sign in to reply to this message.
On Fri, Mar 2, 2012 at 2:52 AM, <philip.liard@gmail.com> wrote: > I see your point Kenton. > > I wish Chrome didn't use extensions too. Sync seems for some reason to > rely quite heavily on them. I don't think I can easily get rid of them. For what it's worth, the sync team agrees on removing them, but since our static initializer infrastructure only counts files with static initializers, that's kind of blocked on protobuf not emitting static initializers for every .proto file. I think the extension stuff could at least be spun off to a different CL. We might be able to work around this in chrome. Nico > > I initially tried to address the extension registration issue in a > separate CL which makes ExtensionIdentifier POD at the same time: > http://codereview.appspot.com/5721046/ > > While I am considering your second approach, I would be grateful if you > could briefly take a look at the changes in extension_set.h and the > comments in cpp_extension.cc and tell me what you think. I think that > this change would > guarantee that extensions are always registered before the user queries > the extension registry. > > > > https://codereview.appspot.com/5643065/diff/27007/src/google/protobuf/extensi... > File src/google/protobuf/extension_set.cc (right): > > https://codereview.appspot.com/5643065/diff/27007/src/google/protobuf/extensi... > src/google/protobuf/extension_set.cc:153: if > (global_state_->is_thread_safe != is_thread_safe) { > On 2012/03/01 20:01:26, Pherl Liu wrote: >> >> Hmm.. So I'm thinking about the pre-built descriptor.pb.cc, which I > > believe >> >> doesn't define extensions currently. It looks like we will not be able > > to define >> >> extension in pre-built protos in the future. > > >> This becomes a more general problem that non-static-init users cannot > > depend on >> >> other libraries that also use protobuf (normally with static init). > > >> How about we keep 2 ExtensionRegistry? I guess the lock-free one > > should take >> >> precedence? > > > Good point. > > About your last sentence mentioning precedence, I might be missing > something. > It seems that I can directly use the right extension registry since in > all cases containing_type is provided, which tells me which extension > registry I have to use. > > https://codereview.appspot.com/5643065/
Sign in to reply to this message.
On Fri, Mar 2, 2012 at 2:52 AM, <philip.liard@gmail.com> wrote: > I see your point Kenton. > > I wish Chrome didn't use extensions too. Sync seems for some reason to > rely quite heavily on them. I don't think I can easily get rid of them. > > I initially tried to address the extension registration issue in a > separate CL which makes ExtensionIdentifier POD at the same time: > http://codereview.appspot.com/**5721046/<http://codereview.appspot.com/5721046/> Sorry, I don't understand how this helps. When a protobuf that contains extensions is parsed, all we see on the wire are tag numbers followed by values. The parsing code looks up each tag in the global extension registry in order to determine how to parse it. If the extension is not registered at that time, it won't be parsed. So say you have code like this: int main(int argc, char* argv[]) { MyProtobufType proto; proto.ParseFromFile(STDIN_FILENO); CHECK(proto.HasExtension(my_extension)); return 0; } With your code, my_extension will not be registered until the HasExtension line, where it is first referenced. However, the proto is parsed on the line before that. If the extension is not registered during parsing, the parser will not recognize it. Therefore, the above code will always CHECK-fail even if the extension is present in the input. Or am I missing some way that you manage to ensure the extension is registered before parsing?
Sign in to reply to this message.
On Fri, Mar 2, 2012 at 9:56 AM, Nico Weber <thakis@chromium.org> wrote: > For what it's worth, the sync team agrees on removing them, but since > our static initializer infrastructure only counts files with static > initializers, that's kind of blocked on protobuf not emitting static > initializers for every .proto file. > > I think the extension stuff could at least be spun off to a different > CL. We might be able to work around this in chrome. > It should be possible to eliminate static initializers for protos that do not declare extensions as a first step.
Sign in to reply to this message.
On 2012/03/02 19:55:08, kenton wrote: > On Fri, Mar 2, 2012 at 9:56 AM, Nico Weber <mailto:thakis@chromium.org> wrote: > > > For what it's worth, the sync team agrees on removing them, but since > > our static initializer infrastructure only counts files with static > > initializers, that's kind of blocked on protobuf not emitting static > > initializers for every .proto file. > > > > I think the extension stuff could at least be spun off to a different > > CL. We might be able to work around this in chrome. > > > > It should be possible to eliminate static initializers for protos that do > not declare extensions as a first step. I sent a new patch set forcing the use of static initializers in non-LITE_RUNTIME mode and when extensions are used. I added a get_default_instance() static method which is simply a getter (no lazy initialization as opposed to default_instance()) rather than generating friend declarations (which didn't seem easy).
Sign in to reply to this message.
get_default_instance() makes me a bit uncomfortable. Two alternative ideas: 1) Call it internal_default_instance(). This at least tells clients that they shouldn't call it. 2) If the default instances were all stored in global variables in an anonymous namespace in the .cc file, then anything else in the .cc file could access them without the need to declare friends, but they'd still be hidden from the public. Since the default_instance() methods aren't inlined anyway, there's no need for the default instance pointers to be declared in the header. So, this approach would be cleaner, but requires more work. Random style comment: > if (!SupportsNoStaticInitializerMode(descriptor_->file())) { This line is really hard to read, because it is a double-negative. Moreover, this makes the else clause a triple-negative. Can you make it like this instead: > if (UseStaticInitializers(descriptor_->file())) { On Tue, Mar 6, 2012 at 8:25 AM, <philip.liard@gmail.com> wrote: > On 2012/03/02 19:55:08, kenton wrote: > >> On Fri, Mar 2, 2012 at 9:56 AM, Nico Weber >> > <mailto:thakis@chromium.org> wrote: > > > For what it's worth, the sync team agrees on removing them, but >> > since > >> > our static initializer infrastructure only counts files with static >> > initializers, that's kind of blocked on protobuf not emitting static >> > initializers for every .proto file. >> > >> > I think the extension stuff could at least be spun off to a >> > different > >> > CL. We might be able to work around this in chrome. >> > >> > > It should be possible to eliminate static initializers for protos that >> > do > >> not declare extensions as a first step. >> > > I sent a new patch set forcing the use of static initializers in > non-LITE_RUNTIME mode and when extensions are used. > > I added a get_default_instance() static method which is simply a getter > (no lazy initialization as opposed to default_instance()) rather than > generating friend declarations (which didn't seem easy). > > https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >
Sign in to reply to this message.
On 2012/03/06 20:32:08, kenton wrote: > get_default_instance() makes me a bit uncomfortable. Two alternative ideas: > > 1) Call it internal_default_instance(). This at least tells clients that > they shouldn't call it. > > 2) If the default instances were all stored in global variables in an > anonymous namespace in the .cc file, then anything else in the .cc file > could access them without the need to declare friends, but they'd still be > hidden from the public. Since the default_instance() methods aren't > inlined anyway, there's no need for the default instance pointers to be > declared in the header. So, this approach would be cleaner, but requires > more work. > > > Random style comment: > > > if (!SupportsNoStaticInitializerMode(descriptor_->file())) { > > This line is really hard to read, because it is a double-negative. > Moreover, this makes the else clause a triple-negative. Can you make it > like this instead: > > > if (UseStaticInitializers(descriptor_->file())) { > > On Tue, Mar 6, 2012 at 8:25 AM, <mailto:philip.liard@gmail.com> wrote: > > > On 2012/03/02 19:55:08, kenton wrote: > > > >> On Fri, Mar 2, 2012 at 9:56 AM, Nico Weber > >> > > <mailto:thakis@chromium.org> wrote: > > > > > For what it's worth, the sync team agrees on removing them, but > >> > > since > > > >> > our static initializer infrastructure only counts files with static > >> > initializers, that's kind of blocked on protobuf not emitting static > >> > initializers for every .proto file. > >> > > >> > I think the extension stuff could at least be spun off to a > >> > > different > > > >> > CL. We might be able to work around this in chrome. > >> > > >> > > > > It should be possible to eliminate static initializers for protos that > >> > > do > > > >> not declare extensions as a first step. > >> > > > > I sent a new patch set forcing the use of static initializers in > > non-LITE_RUNTIME mode and when extensions are used. > > > > I added a get_default_instance() static method which is simply a getter > > (no lazy initialization as opposed to default_instance()) rather than > > generating friend declarations (which didn't seem easy). > > > > > https://codereview.appspot.**com/5643065/%3Chttps://codereview.appspot.com/56...> > > I uploaded a new patch set. I tried the anonymous namespace approach which seemed much cleaner indeed. Unfortunately there is (at least) one case where we need default_instance_ in the header: when Protobuf imports are used. I had a local change which seemed to handle everything including nested types until "make check" generates some code using imports :) Therefore I had to fallback to the first approach and renamed the getter to internal_default_instance(). About "UseStaticInitializers()", I made it "StaticInitializersForced()" instead. "UseStaticInitializers" could suggest that the else clause doesn't use static initializers which is not necessarily true.
Sign in to reply to this message.
On Wed, Mar 7, 2012 at 3:04 AM, <philip.liard@gmail.com> wrote: > I tried the anonymous namespace approach which seemed much cleaner > indeed. Unfortunately there is (at least) one case where we need > default_instance_ in the header: when Protobuf imports are used. > Why are they needed then? You should be able to use default_instance() for inter-file references because imports are guaranteed not to be cyclic, therefore you won't get recursive initialization.
Sign in to reply to this message.
On 2012/03/07 18:35:14, kenton wrote: > On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com> wrote: > > > I tried the anonymous namespace approach which seemed much cleaner > > indeed. Unfortunately there is (at least) one case where we need > > default_instance_ in the header: when Protobuf imports are used. > > > > Why are they needed then? You should be able to use default_instance() for > inter-file references because imports are guaranteed not to be cyclic, > therefore you won't get recursive initialization. Indeed. I sent a new patch set.
Sign in to reply to this message.
Ack, I just noticed that you introduced a bunch of out-of-line calls to default_instance() in various inline accessor methods. In retrospect it is obvious that this would be necessary for your CL, since you want these to be lazily initialized, but it didn't occur to me before. I am concerned that this means these methods will no longer be inlined (because gcc at -O2 will not inline a method if doing so increases overall code size), which could significantly impact performance. Even if they are still inlined, the default_instance() call is now more expensive, although it looks like this only affects getters when the field is not actually set. Unfortunately I'm not entirely sure how to judge the impact of this. Micro-benchmarks probably wouldn't produce realistic performance numbers, and won't tell us anything about code bloat. Any thoughts? Seems we're up to our necks in yak hair with this change. :/ -Kenton On Thu, Mar 8, 2012 at 2:42 AM, <pliard@chromium.org> wrote: > On 2012/03/07 18:35:14, kenton wrote: > > On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com**> wrote: >> > > > I tried the anonymous namespace approach which seemed much cleaner >> > indeed. Unfortunately there is (at least) one case where we need >> > default_instance_ in the header: when Protobuf imports are used. >> > >> > > Why are they needed then? You should be able to use >> > default_instance() for > >> inter-file references because imports are guaranteed not to be cyclic, >> therefore you won't get recursive initialization. >> > > Indeed. I sent a new patch set. > > https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >
Sign in to reply to this message.
A thought: We could trigger default instance initialization in a message object's constructor, and then we would be able to assume in all the accessors that it has been initialized. This, however, requires going back to having the default instance pointers be static members of the classes, at which point we have our original problem again. Declaring all classes in the same file as "friends" should be feasible. Ugly, but perhaps better than having an "internal_default_instance()"? On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda <kenton@google.com> wrote: > Ack, I just noticed that you introduced a bunch of out-of-line calls to > default_instance() in various inline accessor methods. In retrospect it is > obvious that this would be necessary for your CL, since you want these to > be lazily initialized, but it didn't occur to me before. > > I am concerned that this means these methods will no longer be inlined > (because gcc at -O2 will not inline a method if doing so increases overall > code size), which could significantly impact performance. Even if they are > still inlined, the default_instance() call is now more expensive, although > it looks like this only affects getters when the field is not actually set. > > Unfortunately I'm not entirely sure how to judge the impact of this. > Micro-benchmarks probably wouldn't produce realistic performance numbers, > and won't tell us anything about code bloat. Any thoughts? > > Seems we're up to our necks in yak hair with this change. :/ > > -Kenton > > > On Thu, Mar 8, 2012 at 2:42 AM, <pliard@chromium.org> wrote: > >> On 2012/03/07 18:35:14, kenton wrote: >> >> On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com**> wrote: >>> >> >> > I tried the anonymous namespace approach which seemed much cleaner >>> > indeed. Unfortunately there is (at least) one case where we need >>> > default_instance_ in the header: when Protobuf imports are used. >>> > >>> >> >> Why are they needed then? You should be able to use >>> >> default_instance() for >> >>> inter-file references because imports are guaranteed not to be cyclic, >>> therefore you won't get recursive initialization. >>> >> >> Indeed. I sent a new patch set. >> >> https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >> > >
Sign in to reply to this message.
Good point. I might be missing something here. I think that I might have circular dependency issues (again) if I make the constructor call AddDesc() since AddDesc() instantiate the object to set the default instance. Naively, I could fix that by modifying AddDesc() to let it take as a parameter the already allocated instance (this) so that AddDesc() does not need to instantiate it anymore. It might not be that simple though since AddDesc() covers a compilation unit (possibly multiple classes) and not only a single class. That would actually mean making AddDesc() take as many instance parameters as classes we have in the compilation unit (with only one not being null). But, tell me if I'm wrong, providing an already allocated instance (what I have just mentioned) would not work actually since the default instance has to live infinitely thus can't come from the first new() (or even worse, stack allocation) made by the user (who owns the instance). Therefore I don't see how I can avoid the cycle if we invoke AddDesc() in the object's constructor (I might be missing something again). A second approach, or rather a way not to impact users who don't care about static initializers, would be simply to use either default_instance_ or default_instance() in the accessors depending on whether we use static initializers. This is more a workaround but at least it would not impact Google3 (assuming Google3 doesn't care about static initializers) which is probably your first concern. What do you think (before I make any change)? On Mon, Mar 12, 2012 at 9:09 PM, Kenton Varda <kenton@google.com> wrote: > A thought: We could trigger default instance initialization in a message > object's constructor, and then we would be able to assume in all the > accessors that it has been initialized. This, however, requires going back > to having the default instance pointers be static members of the classes, > at which point we have our original problem again. Declaring all classes > in the same file as "friends" should be feasible. Ugly, but perhaps better > than having an "internal_default_instance()"? > > > On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda <kenton@google.com> wrote: > >> Ack, I just noticed that you introduced a bunch of out-of-line calls to >> default_instance() in various inline accessor methods. In retrospect it is >> obvious that this would be necessary for your CL, since you want these to >> be lazily initialized, but it didn't occur to me before. >> >> I am concerned that this means these methods will no longer be inlined >> (because gcc at -O2 will not inline a method if doing so increases overall >> code size), which could significantly impact performance. Even if they are >> still inlined, the default_instance() call is now more expensive, although >> it looks like this only affects getters when the field is not actually set. >> >> Unfortunately I'm not entirely sure how to judge the impact of this. >> Micro-benchmarks probably wouldn't produce realistic performance numbers, >> and won't tell us anything about code bloat. Any thoughts? >> >> Seems we're up to our necks in yak hair with this change. :/ >> >> -Kenton >> >> >> On Thu, Mar 8, 2012 at 2:42 AM, <pliard@chromium.org> wrote: >> >>> On 2012/03/07 18:35:14, kenton wrote: >>> >>> On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com**> >>>> wrote: >>>> >>> >>> > I tried the anonymous namespace approach which seemed much cleaner >>>> > indeed. Unfortunately there is (at least) one case where we need >>>> > default_instance_ in the header: when Protobuf imports are used. >>>> > >>>> >>> >>> Why are they needed then? You should be able to use >>>> >>> default_instance() for >>> >>>> inter-file references because imports are guaranteed not to be cyclic, >>>> therefore you won't get recursive initialization. >>>> >>> >>> Indeed. I sent a new patch set. >>> >>> https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >>> >> >> >
Sign in to reply to this message.
On Tue, Mar 13, 2012 at 9:45 AM, Philippe Liard <pliard@google.com> wrote: > I might be missing something here. I think that I might have circular > dependency issues (again) if I make the constructor call AddDesc() since > AddDesc() instantiate the object to set the default instance. Naively, I > could fix that by modifying AddDesc() to let it take as a parameter the > already allocated instance (this) so that AddDesc() does not need to > instantiate it anymore. > It might not be that simple though since AddDesc() covers a compilation > unit (possibly multiple classes) and not only a single class. That would > actually mean making AddDesc() take as many instance parameters as classes > we have in the compilation unit (with only one not being null). > But, tell me if I'm wrong, providing an already allocated instance (what I > have just mentioned) would not work actually since the default instance has > to live infinitely thus can't come from the first new() (or even worse, > stack allocation) made by the user (who owns the instance). > Therefore I don't see how I can avoid the cycle if we invoke AddDesc() in > the object's constructor (I might be missing something again). > Good point. I feel like there should be a way around this but nothing is coming to mind at the moment. > A second approach, or rather a way not to impact users who don't care > about static initializers, would be simply to use either default_instance_ > or default_instance() in the accessors depending on whether we use static > initializers. This is more a workaround but at least it would not impact > Google3 (assuming Google3 doesn't care about static initializers) which is > probably your first concern. > That sounds acceptable. In that case I think we should be consistent and keep the default_instance_ pointers as static members, with an accessor like "internal_default_instance()" for bypassing the initializer. Perhaps that accessor could only be generated when using lazy initialization. > > What do you think (before I make any change)? > > On Mon, Mar 12, 2012 at 9:09 PM, Kenton Varda <kenton@google.com> wrote: > >> A thought: We could trigger default instance initialization in a message >> object's constructor, and then we would be able to assume in all the >> accessors that it has been initialized. This, however, requires going back >> to having the default instance pointers be static members of the classes, >> at which point we have our original problem again. Declaring all classes >> in the same file as "friends" should be feasible. Ugly, but perhaps better >> than having an "internal_default_instance()"? >> >> >> On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda <kenton@google.com> wrote: >> >>> Ack, I just noticed that you introduced a bunch of out-of-line calls to >>> default_instance() in various inline accessor methods. In retrospect it is >>> obvious that this would be necessary for your CL, since you want these to >>> be lazily initialized, but it didn't occur to me before. >>> >>> I am concerned that this means these methods will no longer be inlined >>> (because gcc at -O2 will not inline a method if doing so increases overall >>> code size), which could significantly impact performance. Even if they are >>> still inlined, the default_instance() call is now more expensive, although >>> it looks like this only affects getters when the field is not actually set. >>> >>> Unfortunately I'm not entirely sure how to judge the impact of this. >>> Micro-benchmarks probably wouldn't produce realistic performance numbers, >>> and won't tell us anything about code bloat. Any thoughts? >>> >>> Seems we're up to our necks in yak hair with this change. :/ >>> >>> -Kenton >>> >>> >>> On Thu, Mar 8, 2012 at 2:42 AM, <pliard@chromium.org> wrote: >>> >>>> On 2012/03/07 18:35:14, kenton wrote: >>>> >>>> On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com**> >>>>> wrote: >>>>> >>>> >>>> > I tried the anonymous namespace approach which seemed much cleaner >>>>> > indeed. Unfortunately there is (at least) one case where we need >>>>> > default_instance_ in the header: when Protobuf imports are used. >>>>> > >>>>> >>>> >>>> Why are they needed then? You should be able to use >>>>> >>>> default_instance() for >>>> >>>>> inter-file references because imports are guaranteed not to be cyclic, >>>>> therefore you won't get recursive initialization. >>>>> >>>> >>>> Indeed. I sent a new patch set. >>>> >>>> https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >>>> >>> >>> >> >
Sign in to reply to this message.
I sent a new patch set. The code generated for users who don't care about static initializers now should be exactly the same as initially. I also did some cleanup and introduced a helper function (PrintHandlingOptionalStaticInitializers()) to avoid repeating some boilerplate code. On Tue, Mar 13, 2012 at 7:16 PM, Kenton Varda <kenton@google.com> wrote: > On Tue, Mar 13, 2012 at 9:45 AM, Philippe Liard <pliard@google.com> wrote: > >> I might be missing something here. I think that I might have circular >> dependency issues (again) if I make the constructor call AddDesc() since >> AddDesc() instantiate the object to set the default instance. Naively, I >> could fix that by modifying AddDesc() to let it take as a parameter the >> already allocated instance (this) so that AddDesc() does not need to >> instantiate it anymore. >> It might not be that simple though since AddDesc() covers a compilation >> unit (possibly multiple classes) and not only a single class. That would >> actually mean making AddDesc() take as many instance parameters as classes >> we have in the compilation unit (with only one not being null). >> But, tell me if I'm wrong, providing an already allocated instance (what >> I have just mentioned) would not work actually since the default instance >> has to live infinitely thus can't come from the first new() (or even worse, >> stack allocation) made by the user (who owns the instance). >> Therefore I don't see how I can avoid the cycle if we invoke AddDesc() in >> the object's constructor (I might be missing something again). >> > > Good point. I feel like there should be a way around this but nothing is > coming to mind at the moment. > > >> A second approach, or rather a way not to impact users who don't care >> about static initializers, would be simply to use either default_instance_ >> or default_instance() in the accessors depending on whether we use static >> initializers. This is more a workaround but at least it would not impact >> Google3 (assuming Google3 doesn't care about static initializers) which is >> probably your first concern. >> > > That sounds acceptable. In that case I think we should be consistent and > keep the default_instance_ pointers as static members, with an accessor > like "internal_default_instance()" for bypassing the initializer. Perhaps > that accessor could only be generated when using lazy initialization. > > >> >> What do you think (before I make any change)? >> >> On Mon, Mar 12, 2012 at 9:09 PM, Kenton Varda <kenton@google.com> wrote: >> >>> A thought: We could trigger default instance initialization in a >>> message object's constructor, and then we would be able to assume in all >>> the accessors that it has been initialized. This, however, requires going >>> back to having the default instance pointers be static members of the >>> classes, at which point we have our original problem again. Declaring all >>> classes in the same file as "friends" should be feasible. Ugly, but >>> perhaps better than having an "internal_default_instance()"? >>> >>> >>> On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda <kenton@google.com> wrote: >>> >>>> Ack, I just noticed that you introduced a bunch of out-of-line calls to >>>> default_instance() in various inline accessor methods. In retrospect it is >>>> obvious that this would be necessary for your CL, since you want these to >>>> be lazily initialized, but it didn't occur to me before. >>>> >>>> I am concerned that this means these methods will no longer be inlined >>>> (because gcc at -O2 will not inline a method if doing so increases overall >>>> code size), which could significantly impact performance. Even if they are >>>> still inlined, the default_instance() call is now more expensive, although >>>> it looks like this only affects getters when the field is not actually set. >>>> >>>> Unfortunately I'm not entirely sure how to judge the impact of this. >>>> Micro-benchmarks probably wouldn't produce realistic performance numbers, >>>> and won't tell us anything about code bloat. Any thoughts? >>>> >>>> Seems we're up to our necks in yak hair with this change. :/ >>>> >>>> -Kenton >>>> >>>> >>>> On Thu, Mar 8, 2012 at 2:42 AM, <pliard@chromium.org> wrote: >>>> >>>>> On 2012/03/07 18:35:14, kenton wrote: >>>>> >>>>> On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com**> >>>>>> wrote: >>>>>> >>>>> >>>>> > I tried the anonymous namespace approach which seemed much cleaner >>>>>> > indeed. Unfortunately there is (at least) one case where we need >>>>>> > default_instance_ in the header: when Protobuf imports are used. >>>>>> > >>>>>> >>>>> >>>>> Why are they needed then? You should be able to use >>>>>> >>>>> default_instance() for >>>>> >>>>>> inter-file references because imports are guaranteed not to be cyclic, >>>>>> therefore you won't get recursive initialization. >>>>>> >>>>> >>>>> Indeed. I sent a new patch set. >>>>> >>>>> https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >>>>> >>>> >>>> >>> >> >
Sign in to reply to this message.
OK, the approach seems good to me now. Thanks for your patience. Pherl should do the full review but I'm happy to consult. It would help a lot if we could see a diff between the old and new generated code for a lite-mode file. Is there some way you could create a separate "code review" that shows this? On Wed, Mar 14, 2012 at 6:58 AM, Philippe Liard <pliard@google.com> wrote: > I sent a new patch set. The code generated for users who don't care about > static initializers now should be exactly the same as initially. I also did > some cleanup and introduced a helper function > (PrintHandlingOptionalStaticInitializers()) to avoid repeating some > boilerplate code. > > > On Tue, Mar 13, 2012 at 7:16 PM, Kenton Varda <kenton@google.com> wrote: > >> On Tue, Mar 13, 2012 at 9:45 AM, Philippe Liard <pliard@google.com>wrote: >> >>> I might be missing something here. I think that I might have circular >>> dependency issues (again) if I make the constructor call AddDesc() since >>> AddDesc() instantiate the object to set the default instance. Naively, I >>> could fix that by modifying AddDesc() to let it take as a parameter the >>> already allocated instance (this) so that AddDesc() does not need to >>> instantiate it anymore. >>> It might not be that simple though since AddDesc() covers a compilation >>> unit (possibly multiple classes) and not only a single class. That would >>> actually mean making AddDesc() take as many instance parameters as classes >>> we have in the compilation unit (with only one not being null). >>> But, tell me if I'm wrong, providing an already allocated instance (what >>> I have just mentioned) would not work actually since the default instance >>> has to live infinitely thus can't come from the first new() (or even worse, >>> stack allocation) made by the user (who owns the instance). >>> Therefore I don't see how I can avoid the cycle if we invoke AddDesc() >>> in the object's constructor (I might be missing something again). >>> >> >> Good point. I feel like there should be a way around this but nothing is >> coming to mind at the moment. >> >> >>> A second approach, or rather a way not to impact users who don't care >>> about static initializers, would be simply to use either default_instance_ >>> or default_instance() in the accessors depending on whether we use static >>> initializers. This is more a workaround but at least it would not impact >>> Google3 (assuming Google3 doesn't care about static initializers) which is >>> probably your first concern. >>> >> >> That sounds acceptable. In that case I think we should be consistent and >> keep the default_instance_ pointers as static members, with an accessor >> like "internal_default_instance()" for bypassing the initializer. Perhaps >> that accessor could only be generated when using lazy initialization. >> >> >>> >>> What do you think (before I make any change)? >>> >>> On Mon, Mar 12, 2012 at 9:09 PM, Kenton Varda <kenton@google.com> wrote: >>> >>>> A thought: We could trigger default instance initialization in a >>>> message object's constructor, and then we would be able to assume in all >>>> the accessors that it has been initialized. This, however, requires going >>>> back to having the default instance pointers be static members of the >>>> classes, at which point we have our original problem again. Declaring all >>>> classes in the same file as "friends" should be feasible. Ugly, but >>>> perhaps better than having an "internal_default_instance()"? >>>> >>>> >>>> On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda <kenton@google.com>wrote: >>>> >>>>> Ack, I just noticed that you introduced a bunch of out-of-line calls >>>>> to default_instance() in various inline accessor methods. In retrospect it >>>>> is obvious that this would be necessary for your CL, since you want these >>>>> to be lazily initialized, but it didn't occur to me before. >>>>> >>>>> I am concerned that this means these methods will no longer be inlined >>>>> (because gcc at -O2 will not inline a method if doing so increases overall >>>>> code size), which could significantly impact performance. Even if they are >>>>> still inlined, the default_instance() call is now more expensive, although >>>>> it looks like this only affects getters when the field is not actually set. >>>>> >>>>> Unfortunately I'm not entirely sure how to judge the impact of this. >>>>> Micro-benchmarks probably wouldn't produce realistic performance numbers, >>>>> and won't tell us anything about code bloat. Any thoughts? >>>>> >>>>> Seems we're up to our necks in yak hair with this change. :/ >>>>> >>>>> -Kenton >>>>> >>>>> >>>>> On Thu, Mar 8, 2012 at 2:42 AM, <pliard@chromium.org> wrote: >>>>> >>>>>> On 2012/03/07 18:35:14, kenton wrote: >>>>>> >>>>>> On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com**> >>>>>>> wrote: >>>>>>> >>>>>> >>>>>> > I tried the anonymous namespace approach which seemed much cleaner >>>>>>> > indeed. Unfortunately there is (at least) one case where we need >>>>>>> > default_instance_ in the header: when Protobuf imports are used. >>>>>>> > >>>>>>> >>>>>> >>>>>> Why are they needed then? You should be able to use >>>>>>> >>>>>> default_instance() for >>>>>> >>>>>>> inter-file references because imports are guaranteed not to be >>>>>>> cyclic, >>>>>>> therefore you won't get recursive initialization. >>>>>>> >>>>>> >>>>>> Indeed. I sent a new patch set. >>>>>> >>>>>> https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >>>>>> >>>>> >>>>> >>>> >>> >> >
Sign in to reply to this message.
Thanks for spending so much time reviewing my CLs! I created a separate CL to see the diffs in the generated LITE_RUNTIME files without extensions: http://codereview.appspot.com/5845046/ Cheers, Philippe. On Thu, Mar 15, 2012 at 10:33 PM, Kenton Varda <kenton@google.com> wrote: > OK, the approach seems good to me now. Thanks for your patience. Pherl > should do the full review but I'm happy to consult. > > It would help a lot if we could see a diff between the old and new > generated code for a lite-mode file. Is there some way you could create a > separate "code review" that shows this? > > On Wed, Mar 14, 2012 at 6:58 AM, Philippe Liard <pliard@google.com> wrote: > >> I sent a new patch set. The code generated for users who don't care about >> static initializers now should be exactly the same as initially. I also did >> some cleanup and introduced a helper function >> (PrintHandlingOptionalStaticInitializers()) to avoid repeating some >> boilerplate code. >> >> >> On Tue, Mar 13, 2012 at 7:16 PM, Kenton Varda <kenton@google.com> wrote: >> >>> On Tue, Mar 13, 2012 at 9:45 AM, Philippe Liard <pliard@google.com>wrote: >>> >>>> I might be missing something here. I think that I might have circular >>>> dependency issues (again) if I make the constructor call AddDesc() since >>>> AddDesc() instantiate the object to set the default instance. Naively, I >>>> could fix that by modifying AddDesc() to let it take as a parameter the >>>> already allocated instance (this) so that AddDesc() does not need to >>>> instantiate it anymore. >>>> It might not be that simple though since AddDesc() covers a compilation >>>> unit (possibly multiple classes) and not only a single class. That would >>>> actually mean making AddDesc() take as many instance parameters as classes >>>> we have in the compilation unit (with only one not being null). >>>> But, tell me if I'm wrong, providing an already allocated instance >>>> (what I have just mentioned) would not work actually since the default >>>> instance has to live infinitely thus can't come from the first new() (or >>>> even worse, stack allocation) made by the user (who owns the instance). >>>> Therefore I don't see how I can avoid the cycle if we invoke AddDesc() >>>> in the object's constructor (I might be missing something again). >>>> >>> >>> Good point. I feel like there should be a way around this but nothing >>> is coming to mind at the moment. >>> >>> >>>> A second approach, or rather a way not to impact users who don't care >>>> about static initializers, would be simply to use either default_instance_ >>>> or default_instance() in the accessors depending on whether we use static >>>> initializers. This is more a workaround but at least it would not impact >>>> Google3 (assuming Google3 doesn't care about static initializers) which is >>>> probably your first concern. >>>> >>> >>> That sounds acceptable. In that case I think we should be consistent >>> and keep the default_instance_ pointers as static members, with an accessor >>> like "internal_default_instance()" for bypassing the initializer. Perhaps >>> that accessor could only be generated when using lazy initialization. >>> >>> >>>> >>>> What do you think (before I make any change)? >>>> >>>> On Mon, Mar 12, 2012 at 9:09 PM, Kenton Varda <kenton@google.com>wrote: >>>> >>>>> A thought: We could trigger default instance initialization in a >>>>> message object's constructor, and then we would be able to assume in all >>>>> the accessors that it has been initialized. This, however, requires going >>>>> back to having the default instance pointers be static members of the >>>>> classes, at which point we have our original problem again. Declaring all >>>>> classes in the same file as "friends" should be feasible. Ugly, but >>>>> perhaps better than having an "internal_default_instance()"? >>>>> >>>>> >>>>> On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda <kenton@google.com>wrote: >>>>> >>>>>> Ack, I just noticed that you introduced a bunch of out-of-line calls >>>>>> to default_instance() in various inline accessor methods. In retrospect it >>>>>> is obvious that this would be necessary for your CL, since you want these >>>>>> to be lazily initialized, but it didn't occur to me before. >>>>>> >>>>>> I am concerned that this means these methods will no longer be >>>>>> inlined (because gcc at -O2 will not inline a method if doing so increases >>>>>> overall code size), which could significantly impact performance. Even if >>>>>> they are still inlined, the default_instance() call is now more expensive, >>>>>> although it looks like this only affects getters when the field is not >>>>>> actually set. >>>>>> >>>>>> Unfortunately I'm not entirely sure how to judge the impact of this. >>>>>> Micro-benchmarks probably wouldn't produce realistic performance numbers, >>>>>> and won't tell us anything about code bloat. Any thoughts? >>>>>> >>>>>> Seems we're up to our necks in yak hair with this change. :/ >>>>>> >>>>>> -Kenton >>>>>> >>>>>> >>>>>> On Thu, Mar 8, 2012 at 2:42 AM, <pliard@chromium.org> wrote: >>>>>> >>>>>>> On 2012/03/07 18:35:14, kenton wrote: >>>>>>> >>>>>>> On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com**> >>>>>>>> wrote: >>>>>>>> >>>>>>> >>>>>>> > I tried the anonymous namespace approach which seemed much cleaner >>>>>>>> > indeed. Unfortunately there is (at least) one case where we need >>>>>>>> > default_instance_ in the header: when Protobuf imports are used. >>>>>>>> > >>>>>>>> >>>>>>> >>>>>>> Why are they needed then? You should be able to use >>>>>>>> >>>>>>> default_instance() for >>>>>>> >>>>>>>> inter-file references because imports are guaranteed not to be >>>>>>>> cyclic, >>>>>>>> therefore you won't get recursive initialization. >>>>>>>> >>>>>>> >>>>>>> Indeed. I sent a new patch set. >>>>>>> >>>>>>> https://codereview.appspot.**com/5643065/<https://codereview.appspot.com/5643... >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
Sign in to reply to this message.
On 2012/03/16 11:05:44, pliard wrote: > Thanks for spending so much time reviewing my CLs! > > I created a separate CL to see the diffs in the generated LITE_RUNTIME > files without extensions: http://codereview.appspot.com/5845046/ > > Cheers, > Philippe. > > On Thu, Mar 15, 2012 at 10:33 PM, Kenton Varda <mailto:kenton@google.com> wrote: > > > OK, the approach seems good to me now. Thanks for your patience. Pherl > > should do the full review but I'm happy to consult. > > > > It would help a lot if we could see a diff between the old and new > > generated code for a lite-mode file. Is there some way you could create a > > separate "code review" that shows this? > > > > On Wed, Mar 14, 2012 at 6:58 AM, Philippe Liard <mailto:pliard@google.com> wrote: > > > >> I sent a new patch set. The code generated for users who don't care about > >> static initializers now should be exactly the same as initially. I also did > >> some cleanup and introduced a helper function > >> (PrintHandlingOptionalStaticInitializers()) to avoid repeating some > >> boilerplate code. > >> > >> > >> On Tue, Mar 13, 2012 at 7:16 PM, Kenton Varda <mailto:kenton@google.com> wrote: > >> > >>> On Tue, Mar 13, 2012 at 9:45 AM, Philippe Liard <pliard@google.com>wrote: > >>> > >>>> I might be missing something here. I think that I might have circular > >>>> dependency issues (again) if I make the constructor call AddDesc() since > >>>> AddDesc() instantiate the object to set the default instance. Naively, I > >>>> could fix that by modifying AddDesc() to let it take as a parameter the > >>>> already allocated instance (this) so that AddDesc() does not need to > >>>> instantiate it anymore. > >>>> It might not be that simple though since AddDesc() covers a compilation > >>>> unit (possibly multiple classes) and not only a single class. That would > >>>> actually mean making AddDesc() take as many instance parameters as classes > >>>> we have in the compilation unit (with only one not being null). > >>>> But, tell me if I'm wrong, providing an already allocated instance > >>>> (what I have just mentioned) would not work actually since the default > >>>> instance has to live infinitely thus can't come from the first new() (or > >>>> even worse, stack allocation) made by the user (who owns the instance). > >>>> Therefore I don't see how I can avoid the cycle if we invoke AddDesc() > >>>> in the object's constructor (I might be missing something again). > >>>> > >>> > >>> Good point. I feel like there should be a way around this but nothing > >>> is coming to mind at the moment. > >>> > >>> > >>>> A second approach, or rather a way not to impact users who don't care > >>>> about static initializers, would be simply to use either default_instance_ > >>>> or default_instance() in the accessors depending on whether we use static > >>>> initializers. This is more a workaround but at least it would not impact > >>>> Google3 (assuming Google3 doesn't care about static initializers) which is > >>>> probably your first concern. > >>>> > >>> > >>> That sounds acceptable. In that case I think we should be consistent > >>> and keep the default_instance_ pointers as static members, with an accessor > >>> like "internal_default_instance()" for bypassing the initializer. Perhaps > >>> that accessor could only be generated when using lazy initialization. > >>> > >>> > >>>> > >>>> What do you think (before I make any change)? > >>>> > >>>> On Mon, Mar 12, 2012 at 9:09 PM, Kenton Varda <kenton@google.com>wrote: > >>>> > >>>>> A thought: We could trigger default instance initialization in a > >>>>> message object's constructor, and then we would be able to assume in all > >>>>> the accessors that it has been initialized. This, however, requires going > >>>>> back to having the default instance pointers be static members of the > >>>>> classes, at which point we have our original problem again. Declaring all > >>>>> classes in the same file as "friends" should be feasible. Ugly, but > >>>>> perhaps better than having an "internal_default_instance()"? > >>>>> > >>>>> > >>>>> On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda <kenton@google.com>wrote: > >>>>> > >>>>>> Ack, I just noticed that you introduced a bunch of out-of-line calls > >>>>>> to default_instance() in various inline accessor methods. In retrospect > it > >>>>>> is obvious that this would be necessary for your CL, since you want these > >>>>>> to be lazily initialized, but it didn't occur to me before. > >>>>>> > >>>>>> I am concerned that this means these methods will no longer be > >>>>>> inlined (because gcc at -O2 will not inline a method if doing so > increases > >>>>>> overall code size), which could significantly impact performance. Even > if > >>>>>> they are still inlined, the default_instance() call is now more > expensive, > >>>>>> although it looks like this only affects getters when the field is not > >>>>>> actually set. > >>>>>> > >>>>>> Unfortunately I'm not entirely sure how to judge the impact of this. > >>>>>> Micro-benchmarks probably wouldn't produce realistic performance > numbers, > >>>>>> and won't tell us anything about code bloat. Any thoughts? > >>>>>> > >>>>>> Seems we're up to our necks in yak hair with this change. :/ > >>>>>> > >>>>>> -Kenton > >>>>>> > >>>>>> > >>>>>> On Thu, Mar 8, 2012 at 2:42 AM, <mailto:pliard@chromium.org> wrote: > >>>>>> > >>>>>>> On 2012/03/07 18:35:14, kenton wrote: > >>>>>>> > >>>>>>> On Wed, Mar 7, 2012 at 3:04 AM, <mailto:philip.liard@gmail.com**> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>> > >>>>>>> > I tried the anonymous namespace approach which seemed much cleaner > >>>>>>>> > indeed. Unfortunately there is (at least) one case where we need > >>>>>>>> > default_instance_ in the header: when Protobuf imports are used. > >>>>>>>> > > >>>>>>>> > >>>>>>> > >>>>>>> Why are they needed then? You should be able to use > >>>>>>>> > >>>>>>> default_instance() for > >>>>>>> > >>>>>>>> inter-file references because imports are guaranteed not to be > >>>>>>>> cyclic, > >>>>>>>> therefore you won't get recursive initialization. > >>>>>>>> > >>>>>>> > >>>>>>> Indeed. I sent a new patch set. > >>>>>>> > >>>>>>> > https://codereview.appspot.**com/5643065/%3Chttps://codereview.appspot.com/56...> > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > We still have this CL :)
Sign in to reply to this message.
Oops.. There'll be an offsite for the rest of week. I'll try to review the CL during the offsite.. On Tue, Apr 10, 2012 at 4:19 PM, <pliard@chromium.org> wrote: > On 2012/03/16 11:05:44, pliard wrote: > >> Thanks for spending so much time reviewing my CLs! >> > > I created a separate CL to see the diffs in the generated LITE_RUNTIME >> files without extensions: http://codereview.appspot.com/**5845046/<http://codereview.appspot.com/5845046/> >> > > Cheers, >> Philippe. >> > > On Thu, Mar 15, 2012 at 10:33 PM, Kenton Varda >> > <mailto:kenton@google.com> wrote: > > > OK, the approach seems good to me now. Thanks for your patience. >> > Pherl > >> > should do the full review but I'm happy to consult. >> > >> > It would help a lot if we could see a diff between the old and new >> > generated code for a lite-mode file. Is there some way you could >> > create a > >> > separate "code review" that shows this? >> > >> > On Wed, Mar 14, 2012 at 6:58 AM, Philippe Liard >> > <mailto:pliard@google.com> wrote: > >> > >> >> I sent a new patch set. The code generated for users who don't care >> > about > >> >> static initializers now should be exactly the same as initially. I >> > also did > >> >> some cleanup and introduced a helper function >> >> (**PrintHandlingOptionalStaticIni**tializers()) to avoid repeating >> some >> >> boilerplate code. >> >> >> >> >> >> On Tue, Mar 13, 2012 at 7:16 PM, Kenton Varda >> > <mailto:kenton@google.com> wrote: > >> >> >> >>> On Tue, Mar 13, 2012 at 9:45 AM, Philippe Liard >> > <pliard@google.com>wrote: > >> >>> >> >>>> I might be missing something here. I think that I might have >> > circular > >> >>>> dependency issues (again) if I make the constructor call >> > AddDesc() since > >> >>>> AddDesc() instantiate the object to set the default instance. >> > Naively, I > >> >>>> could fix that by modifying AddDesc() to let it take as a >> > parameter the > >> >>>> already allocated instance (this) so that AddDesc() does not need >> > to > >> >>>> instantiate it anymore. >> >>>> It might not be that simple though since AddDesc() covers a >> > compilation > >> >>>> unit (possibly multiple classes) and not only a single class. >> > That would > >> >>>> actually mean making AddDesc() take as many instance parameters >> > as classes > >> >>>> we have in the compilation unit (with only one not being null). >> >>>> But, tell me if I'm wrong, providing an already allocated >> > instance > >> >>>> (what I have just mentioned) would not work actually since the >> > default > >> >>>> instance has to live infinitely thus can't come from the first >> > new() (or > >> >>>> even worse, stack allocation) made by the user (who owns the >> > instance). > >> >>>> Therefore I don't see how I can avoid the cycle if we invoke >> > AddDesc() > >> >>>> in the object's constructor (I might be missing something again). >> >>>> >> >>> >> >>> Good point. I feel like there should be a way around this but >> > nothing > >> >>> is coming to mind at the moment. >> >>> >> >>> >> >>>> A second approach, or rather a way not to impact users who don't >> > care > >> >>>> about static initializers, would be simply to use either >> > default_instance_ > >> >>>> or default_instance() in the accessors depending on whether we >> > use static > >> >>>> initializers. This is more a workaround but at least it would not >> > impact > >> >>>> Google3 (assuming Google3 doesn't care about static initializers) >> > which is > >> >>>> probably your first concern. >> >>>> >> >>> >> >>> That sounds acceptable. In that case I think we should be >> > consistent > >> >>> and keep the default_instance_ pointers as static members, with an >> > accessor > >> >>> like "internal_default_instance()" for bypassing the initializer. >> > Perhaps > >> >>> that accessor could only be generated when using lazy >> > initialization. > >> >>> >> >>> >> >>>> >> >>>> What do you think (before I make any change)? >> >>>> >> >>>> On Mon, Mar 12, 2012 at 9:09 PM, Kenton Varda >> > <kenton@google.com>wrote: > >> >>>> >> >>>>> A thought: We could trigger default instance initialization in >> > a > >> >>>>> message object's constructor, and then we would be able to >> > assume in all > >> >>>>> the accessors that it has been initialized. This, however, >> > requires going > >> >>>>> back to having the default instance pointers be static members >> > of the > >> >>>>> classes, at which point we have our original problem again. >> > Declaring all > >> >>>>> classes in the same file as "friends" should be feasible. Ugly, >> > but > >> >>>>> perhaps better than having an "internal_default_instance()"? >> >>>>> >> >>>>> >> >>>>> On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda >> > <kenton@google.com>wrote: > >> >>>>> >> >>>>>> Ack, I just noticed that you introduced a bunch of out-of-line >> > calls > >> >>>>>> to default_instance() in various inline accessor methods. In >> > retrospect > >> it >> >>>>>> is obvious that this would be necessary for your CL, since you >> > want these > >> >>>>>> to be lazily initialized, but it didn't occur to me before. >> >>>>>> >> >>>>>> I am concerned that this means these methods will no longer be >> >>>>>> inlined (because gcc at -O2 will not inline a method if doing >> > so > >> increases >> >>>>>> overall code size), which could significantly impact >> > performance. Even > >> if >> >>>>>> they are still inlined, the default_instance() call is now more >> expensive, >> >>>>>> although it looks like this only affects getters when the field >> > is not > >> >>>>>> actually set. >> >>>>>> >> >>>>>> Unfortunately I'm not entirely sure how to judge the impact of >> > this. > >> >>>>>> Micro-benchmarks probably wouldn't produce realistic >> > performance > >> numbers, >> >>>>>> and won't tell us anything about code bloat. Any thoughts? >> >>>>>> >> >>>>>> Seems we're up to our necks in yak hair with this change. :/ >> >>>>>> >> >>>>>> -Kenton >> >>>>>> >> >>>>>> >> >>>>>> On Thu, Mar 8, 2012 at 2:42 AM, <mailto:pliard@chromium.org> >> > wrote: > >> >>>>>> >> >>>>>>> On 2012/03/07 18:35:14, kenton wrote: >> >>>>>>> >> >>>>>>> On Wed, Mar 7, 2012 at 3:04 AM, >> > <mailto:philip.liard@gmail.com****> > >> >>>>>>>> wrote: >> >>>>>>>> >> >>>>>>> >> >>>>>>> > I tried the anonymous namespace approach which seemed much >> > cleaner > >> >>>>>>>> > indeed. Unfortunately there is (at least) one case where we >> > need > >> >>>>>>>> > default_instance_ in the header: when Protobuf imports are >> > used. > >> >>>>>>>> > >> >>>>>>>> >> >>>>>>> >> >>>>>>> Why are they needed then? You should be able to use >> >>>>>>>> >> >>>>>>> default_instance() for >> >>>>>>> >> >>>>>>>> inter-file references because imports are guaranteed not to >> > be > >> >>>>>>>> cyclic, >> >>>>>>>> therefore you won't get recursive initialization. >> >>>>>>>> >> >>>>>>> >> >>>>>>> Indeed. I sent a new patch set. >> >>>>>>> >> >>>>>>> >> > > https://codereview.appspot.****com/5643065/%3Chttps://coderev** > iew.appspot.com/5643065/ <http://codereview.appspot.com/5643065/>> > >> >>>>>>> >> >>>>>> >> >>>>>> >> >>>>> >> >>>> >> >>> >> >> >> > >> > > We still have this CL :) > > http://codereview.appspot.com/**5643065/<http://codereview.appspot.com/5643065/> >
Sign in to reply to this message.
No worries :) On Tue, Apr 10, 2012 at 12:25 PM, Pherl Liu <liujisi@google.com> wrote: > Oops.. There'll be an offsite for the rest of week. I'll try to review the > CL during the offsite.. > > > On Tue, Apr 10, 2012 at 4:19 PM, <pliard@chromium.org> wrote: > >> On 2012/03/16 11:05:44, pliard wrote: >> >>> Thanks for spending so much time reviewing my CLs! >>> >> >> I created a separate CL to see the diffs in the generated LITE_RUNTIME >>> files without extensions: http://codereview.appspot.com/**5845046/<http://codereview.appspot.com/5845046/> >>> >> >> Cheers, >>> Philippe. >>> >> >> On Thu, Mar 15, 2012 at 10:33 PM, Kenton Varda >>> >> <mailto:kenton@google.com> wrote: >> >> > OK, the approach seems good to me now. Thanks for your patience. >>> >> Pherl >> >>> > should do the full review but I'm happy to consult. >>> > >>> > It would help a lot if we could see a diff between the old and new >>> > generated code for a lite-mode file. Is there some way you could >>> >> create a >> >>> > separate "code review" that shows this? >>> > >>> > On Wed, Mar 14, 2012 at 6:58 AM, Philippe Liard >>> >> <mailto:pliard@google.com> wrote: >> >>> > >>> >> I sent a new patch set. The code generated for users who don't care >>> >> about >> >>> >> static initializers now should be exactly the same as initially. I >>> >> also did >> >>> >> some cleanup and introduced a helper function >>> >> (**PrintHandlingOptionalStaticIni**tializers()) to avoid repeating >>> some >>> >> boilerplate code. >>> >> >>> >> >>> >> On Tue, Mar 13, 2012 at 7:16 PM, Kenton Varda >>> >> <mailto:kenton@google.com> wrote: >> >>> >> >>> >>> On Tue, Mar 13, 2012 at 9:45 AM, Philippe Liard >>> >> <pliard@google.com>wrote: >> >>> >>> >>> >>>> I might be missing something here. I think that I might have >>> >> circular >> >>> >>>> dependency issues (again) if I make the constructor call >>> >> AddDesc() since >> >>> >>>> AddDesc() instantiate the object to set the default instance. >>> >> Naively, I >> >>> >>>> could fix that by modifying AddDesc() to let it take as a >>> >> parameter the >> >>> >>>> already allocated instance (this) so that AddDesc() does not need >>> >> to >> >>> >>>> instantiate it anymore. >>> >>>> It might not be that simple though since AddDesc() covers a >>> >> compilation >> >>> >>>> unit (possibly multiple classes) and not only a single class. >>> >> That would >> >>> >>>> actually mean making AddDesc() take as many instance parameters >>> >> as classes >> >>> >>>> we have in the compilation unit (with only one not being null). >>> >>>> But, tell me if I'm wrong, providing an already allocated >>> >> instance >> >>> >>>> (what I have just mentioned) would not work actually since the >>> >> default >> >>> >>>> instance has to live infinitely thus can't come from the first >>> >> new() (or >> >>> >>>> even worse, stack allocation) made by the user (who owns the >>> >> instance). >> >>> >>>> Therefore I don't see how I can avoid the cycle if we invoke >>> >> AddDesc() >> >>> >>>> in the object's constructor (I might be missing something again). >>> >>>> >>> >>> >>> >>> Good point. I feel like there should be a way around this but >>> >> nothing >> >>> >>> is coming to mind at the moment. >>> >>> >>> >>> >>> >>>> A second approach, or rather a way not to impact users who don't >>> >> care >> >>> >>>> about static initializers, would be simply to use either >>> >> default_instance_ >> >>> >>>> or default_instance() in the accessors depending on whether we >>> >> use static >> >>> >>>> initializers. This is more a workaround but at least it would not >>> >> impact >> >>> >>>> Google3 (assuming Google3 doesn't care about static initializers) >>> >> which is >> >>> >>>> probably your first concern. >>> >>>> >>> >>> >>> >>> That sounds acceptable. In that case I think we should be >>> >> consistent >> >>> >>> and keep the default_instance_ pointers as static members, with an >>> >> accessor >> >>> >>> like "internal_default_instance()" for bypassing the initializer. >>> >> Perhaps >> >>> >>> that accessor could only be generated when using lazy >>> >> initialization. >> >>> >>> >>> >>> >>> >>>> >>> >>>> What do you think (before I make any change)? >>> >>>> >>> >>>> On Mon, Mar 12, 2012 at 9:09 PM, Kenton Varda >>> >> <kenton@google.com>wrote: >> >>> >>>> >>> >>>>> A thought: We could trigger default instance initialization in >>> >> a >> >>> >>>>> message object's constructor, and then we would be able to >>> >> assume in all >> >>> >>>>> the accessors that it has been initialized. This, however, >>> >> requires going >> >>> >>>>> back to having the default instance pointers be static members >>> >> of the >> >>> >>>>> classes, at which point we have our original problem again. >>> >> Declaring all >> >>> >>>>> classes in the same file as "friends" should be feasible. Ugly, >>> >> but >> >>> >>>>> perhaps better than having an "internal_default_instance()"? >>> >>>>> >>> >>>>> >>> >>>>> On Mon, Mar 12, 2012 at 2:00 PM, Kenton Varda >>> >> <kenton@google.com>wrote: >> >>> >>>>> >>> >>>>>> Ack, I just noticed that you introduced a bunch of out-of-line >>> >> calls >> >>> >>>>>> to default_instance() in various inline accessor methods. In >>> >> retrospect >> >>> it >>> >>>>>> is obvious that this would be necessary for your CL, since you >>> >> want these >> >>> >>>>>> to be lazily initialized, but it didn't occur to me before. >>> >>>>>> >>> >>>>>> I am concerned that this means these methods will no longer be >>> >>>>>> inlined (because gcc at -O2 will not inline a method if doing >>> >> so >> >>> increases >>> >>>>>> overall code size), which could significantly impact >>> >> performance. Even >> >>> if >>> >>>>>> they are still inlined, the default_instance() call is now more >>> expensive, >>> >>>>>> although it looks like this only affects getters when the field >>> >> is not >> >>> >>>>>> actually set. >>> >>>>>> >>> >>>>>> Unfortunately I'm not entirely sure how to judge the impact of >>> >> this. >> >>> >>>>>> Micro-benchmarks probably wouldn't produce realistic >>> >> performance >> >>> numbers, >>> >>>>>> and won't tell us anything about code bloat. Any thoughts? >>> >>>>>> >>> >>>>>> Seems we're up to our necks in yak hair with this change. :/ >>> >>>>>> >>> >>>>>> -Kenton >>> >>>>>> >>> >>>>>> >>> >>>>>> On Thu, Mar 8, 2012 at 2:42 AM, <mailto:pliard@chromium.org> >>> >> wrote: >> >>> >>>>>> >>> >>>>>>> On 2012/03/07 18:35:14, kenton wrote: >>> >>>>>>> >>> >>>>>>> On Wed, Mar 7, 2012 at 3:04 AM, >>> >> <mailto:philip.liard@gmail.com****> >> >>> >>>>>>>> wrote: >>> >>>>>>>> >>> >>>>>>> >>> >>>>>>> > I tried the anonymous namespace approach which seemed much >>> >> cleaner >> >>> >>>>>>>> > indeed. Unfortunately there is (at least) one case where we >>> >> need >> >>> >>>>>>>> > default_instance_ in the header: when Protobuf imports are >>> >> used. >> >>> >>>>>>>> > >>> >>>>>>>> >>> >>>>>>> >>> >>>>>>> Why are they needed then? You should be able to use >>> >>>>>>>> >>> >>>>>>> default_instance() for >>> >>>>>>> >>> >>>>>>>> inter-file references because imports are guaranteed not to >>> >> be >> >>> >>>>>>>> cyclic, >>> >>>>>>>> therefore you won't get recursive initialization. >>> >>>>>>>> >>> >>>>>>> >>> >>>>>>> Indeed. I sent a new patch set. >>> >>>>>>> >>> >>>>>>> >>> >> >> https://codereview.appspot.****com/5643065/%3Chttps://coderev** >> iew.appspot.com/5643065/ <http://codereview.appspot.com/5643065/>> >> >>> >>>>>>> >>> >>>>>> >>> >>>>>> >>> >>>>> >>> >>>> >>> >>> >>> >> >>> > >>> >> >> We still have this CL :) >> >> http://codereview.appspot.com/**5643065/<http://codereview.appspot.com/5643065/> >> > >
Sign in to reply to this message.
Can you attach a diff for the generated protobuf-lite messages? Also, maybe add a test for the protobuf-lite default instance. http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... File src/google/protobuf/compiler/cpp/cpp_file.cc (right): http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_file.cc:510: "void $adddescriptorsname$_impl() {\n" add "static" to make this internal? http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... File src/google/protobuf/compiler/cpp/cpp_helpers.h (right): http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_helpers.h:161: return HasDescriptorMethods(file) || file->extension_count() > 0; what about the extensions defined in messages (not top-level extensions)? http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... File src/google/protobuf/compiler/cpp/cpp_message.cc (right): http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_message.cc:499: "// Returns the internal default instance pointer. This function can return\n" 80 chars http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_message.cc:1064: " $name$_ = const_cast< $type$*>($type$::internal_default_instance());\n", 80 chars
Sign in to reply to this message.
Hi Pherl, I addressed your comments. Regarding the diff presenting the impact of this change, I included a link to another CL in one of my previous comments: http://codereview.appspot.com/5845046/ About the unit tests, we have some lite-runtime code generated (unittest_import_lite.pb.* and unittest_lite_imports_nonlite.pb.*) which includes the #ifdefs generated by this CL. My understanding is that these files are compiled but not used by any unit test (i.e. we test the compiler but not the behavior of the generated code ). It seems that I can hardly change that. Thanks, Philippe. http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... File src/google/protobuf/compiler/cpp/cpp_file.cc (right): http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_file.cc:510: "void $adddescriptorsname$_impl() {\n" On 2012/04/16 07:58:04, Pherl Liu wrote: > add "static" to make this internal? That's not obvious but this function needs to be declared in the header as "friend" for the same reasons as $adddescriptorsname$ (need to access private static variables like default_instance_). We need to have two functions ($adddescriptorsname$ and $$adddescriptorsname$_impl) since CallOnce() takes a function pointer. http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... File src/google/protobuf/compiler/cpp/cpp_helpers.h (right): http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_helpers.h:161: return HasDescriptorMethods(file) || file->extension_count() > 0; On 2012/04/16 07:58:04, Pherl Liu wrote: > what about the extensions defined in messages (not top-level extensions)? Good point! I moved the new implementation to the cc file. http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... File src/google/protobuf/compiler/cpp/cpp_message.cc (right): http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_message.cc:499: "// Returns the internal default instance pointer. This function can return\n" On 2012/04/16 07:58:04, Pherl Liu wrote: > 80 chars Done. http://codereview.appspot.com/5643065/diff/76001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_message.cc:1064: " $name$_ = const_cast< $type$*>($type$::internal_default_instance());\n", On 2012/04/16 07:58:04, Pherl Liu wrote: > 80 chars Done.
Sign in to reply to this message.
LGTM Thanks for the work! One comments about the HasExtensions, otherwise LG http://codereview.appspot.com/5643065/diff/92001/src/google/protobuf/compiler... File src/google/protobuf/compiler/cpp/cpp_helpers.cc (right): http://codereview.appspot.com/5643065/diff/92001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_helpers.cc:364: for (int i = 0; i < file->message_type_count(); ++i) { hmm, then looks like the file level extensions are ignored...
Sign in to reply to this message.
Thanks for the review Pherl! I'm waiting for your final LGTM before I commit this. http://codereview.appspot.com/5643065/diff/92001/src/google/protobuf/compiler... File src/google/protobuf/compiler/cpp/cpp_helpers.cc (right): http://codereview.appspot.com/5643065/diff/92001/src/google/protobuf/compiler... src/google/protobuf/compiler/cpp/cpp_helpers.cc:364: for (int i = 0; i < file->message_type_count(); ++i) { On 2012/05/04 06:26:00, Pherl Liu wrote: > hmm, then looks like the file level extensions are ignored... Done.
Sign in to reply to this message.
|