[libcxx-commits] [PATCH] D68617: partially inline copy constructor basic_string(const basic_string&[, allocator])

[Martijn Vels via Phabricator via libcxx-commits]

mvels marked an inline comment as done.
mvels added inline comments.


================
Comment at: libcxx/include/__config:766
+#define _LIBCPP_HIDE_FROM_ABI_IN_UNSTABLE _LIBCPP_HIDE_FROM_ABI
+#define _LIBCPP_INLINE_IN_UNSTABLE inline
+#else
----------------
ldionne wrote:
> mvels wrote:
> > ldionne wrote:
> > > ldionne wrote:
> > > > I'm really uncomfortable with piggy-backing on the fact that non-inline member functions with an explicit instantiation declaration won't be inlined into user code. We're also excluding this optimization from most builds which are not using the unstable ABI.
> > > > 
> > > > I think it's possible to do better by encoding the knowledge of when `__init_long()` was added to the shared library. As far as you're concerned, you could basically write code using `_LIBCPP_HAS_INIT_LONG_IN_DYLIB` (or whatever), and then vendors could go and define that macro when suitable for them.
> > > > 
> > > > We could also define all these `_LIBCPP_HAS_<function-name>_IN_DYLIB` macros in case we're using the unstable ABI. WDYT?
> > > So, basically, my suggestion would be this:
> > > 
> > > 1. `#define _LIBCPP_HAS_INIT_LONG_IN_DYLIB` in `__config` only when `_LIBCPP_ABI_UNSTABLE` is defined
> > > 2. Write the code in `<string>` such that it takes advantage of `__init_long` when it _is_in the dylib, and it doesn't when it's not (but in both cases the code should work).
> > > 3. I'll then go and also define `_LIBCPP_HAS_INIT_LONG_IN_DYLIB` when it makes sense based on the deployment target, and other vendors can do the same.
> > > 
> > > This seems like a clean solution where libc++ ends up being optimal in all cases where it can be. This also introduces a structured way of making this sort of change.
> > >I'm really uncomfortable with piggy-backing on the fact that non-inline member functions with an explicit instantiation declaration won't be inlined into user code. We're also excluding this optimization from most builds which are not using the unstable ABI.
> > 
> > These defines are primarily intended to say 'exclude from ABI yes/no for Stable/Unstable. We may not need the 'inline' hint, I basically added this following existing definitions in string for inline visibility. I may also miss a finer point you are making, I understand that clang now never considers external template methods for inlining, which I understand is WAD? Even so, we could keep the ctor in the ABI and add 'inline', but then we still have the 'may call methods introduced later' ABI break.
> > 
> > > _LIBCPP_HAS_INIT_LONG_IN_DYLIB
> > I have 2 main concerns here
> > 
> > 1. this requires that vendors / clients have full control over all their dependencies and used dylib versions, which are defined at compilation time with little control at deployment and runtime. If I use some 'foo.so' from some other team or vendor, then this has repercussions on which libc++ I use or deploy. We basically move the 'you may break ABI' decision elsewhere in a 'per case' basis?
> > 
> > 2. I have a good couple similar small inlining opportunities which all run into the same ABI dillema, I don't think long list of method ABI inclusions are desirable, and I am concerned this 'selectively pick you ABI compatibility is a safe long term option. I really like the idea that you have 'stable' (and no surprises) or 'unstable', and you manage your full build , dependency and deployment story (which does apply to Google).
> > 
> > >This seems like a clean solution where libc++ ends up being optimal in all cases where it can be. This also introduces a structured way of making this sort of change
> > I think we are on similar paths on 'allow this optimization where possible / safe'. Perhaps we divert on what 'safe' is in this context. 
> > 
> > My 'safety' concern scenario would be some team X providing some foo.so to various other teams. When does team X safely turn this __INIT_LONG on? It can't safely do this IMHO, we basically make it a risk factor for team X, and they may not even realize the implications. This is only safe in the context of an explicit LIBCPP version, or using an explicit 'unstable' libc++.
> > These defines are primarily intended to say 'exclude from ABI yes/no for Stable/Unstable.
> 
> My main problem with using `_LIBCPP_ABI_UNSTABLE` as the _sole_ determinant for whether to perform these optimizations is that most vendors are not on the unstable ABI. `_LIBCPP_ABI_UNSTABLE` is a bigger gun than we need to use here, and the result is that many users that _could_ get the optimization will not, simply because they are on the stable ABI.
> 
> > I have a good couple similar small inlining opportunities which all run into the same ABI dillema
> 
> If they are all semantically related, I think it makes sense to hide them behind the same macro (kind of like `_LIBCPP_DO_NOT_ASSUME_STREAMS_EXPLICIT_INSTANTIATION_IN_DYLIB`, which doesn't list each and every stream method).
> 
> I also think that class-wide explicit instantiations are somewhat greedy in that they end up including a lot more things in the dylib than we might want. For example, do we really mean to explicitly instantiate `std::basic_string<wchar_t>::__init_long()` in the dylib, or would the one for `std::basic_string<char>` be sufficient? I guess the point I'm trying to make is that everything we put in the dylib should be well reasoned, and as a corollary there should be few things in the dylib. So there should also be only few macros like `_LIBCPP_HAS_INIT_LONG_IN_DYLIB ` that need to exist.
> 
> > I think we are on similar paths on 'allow this optimization where possible / safe'. Perhaps we divert on what 'safe' is in this context.
> 
> I think we might be very close to an agreement, but perhaps I did not properly explain the solution I propose. So, basically, what I'm saying is to make the code work whether `__init_long()` is in the dylib or not, and don't tie that decision to whether we're using the unstable ABI. Then, through the `-target` option (or `-mmacosx-version-min` and similar switches), the user tells the compiler which target they want to deploy to. The compiler sets some macros based on that, and libc++ checks those macros to see whether the dylib on the desired target contains the required functionality. If no information about the deployment target is known (e.g. for vendors that don't collect that information), we can assume that the functionality isn't there to be conservative. If we're using the unstable ABI, on the other hand, we can assume that the dylib contains the required functionality regardless of the deployment target.
> 
> This all happens in the headers on the __user__ side, i.e. when building an application that needs to link against `libc++.dylib`. This is, in essence, a superset of what you're proposing with the stable/unstable ABI solution, but it allows the majority of users who (1) are using the stable ABI but (2) only need to deploy to recent targets that contain `__init_long()` to benefit from the optimization.
> 
> If the default is to be conservative (and not assume `__init_long` in the dylib) unless we have enough information to tell otherwise, I don't see how my approach can be unsafe (but perhaps I missed something).
>_LIBCPP_ABI_UNSTABLE is a bigger gun than we need to use here, and the result is that many users that _could_ get the optimization will not, simply because they are on the stable ABI.

I agree it is a very big gun. This comes from my understanding that the ABI is a 'big thing', but I'll happily defer to feature based enabling, (i.e., selective ABI breaking)

Re naming, I would personally find 'use optimized ctor' the feature to select, __init_long() is a requirement for that, I don't see it directly as 'rthe feature' ?

I.e.:

```
// Inline copy constructor code for small strings.
// Requires the presence of '__init_long' in lib/ dylib
#define _LIBCPP_ABI_INLINE_SMALL_COPY_CTOR
```

versus

```
// Assume the presence of __init_long in the library / dylib, allowing for 
// SSO optimizations and inlining of the copy constructor.
#define _LIBCPP_ABI_INLINE_SMALL_COPY_CTOR
```

I could be at peace with both, my discomfort with the latter is that it does not tell me directly what we are actually turning on.  One further complication is where we have dual requirements. For example, some other optimization requires A and B, meaning, enabling A and B as separate features triggering the real feature we actually want. 

And then there is the fact that clang aggressively inlines non ABI functions, which is undesirable. Which is why I crafted the changes here to have __init_long() only inline if the ctor is in the ABI to avoid expanding the size of inlined code.

I am leaning towards '_LIBCPP_ABI_INLINE_SMALL_COPY_CTOR' (and subsequent small ABI breakages) in that case. Alternatively, name these '_LIBCPP_ABI_INLINE_SSO_OPERATIONS' which would cover current and future optimizations. Although this come close to 'allow any ABI breakage, now and future, i.e. unstable).

Condensed: is there use case for people want 'some unstable, but not all'? I honestly don't know an answer there :)  This is a wide topic, I very much appreciate your feedback on this.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68617/new/

https://reviews.llvm.org/D68617