[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:
> 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++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68617