[PATCH] D70600: [Error] Add stack traces for llvm::Error invariant violations.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D70600#1802516 <https://reviews.llvm.org/D70600#1802516>, @sammccall wrote:

> In D70600#1802227 <https://reviews.llvm.org/D70600#1802227>, @lhames wrote:
>
> > In D70600#1801726 <https://reviews.llvm.org/D70600#1801726>, @sammccall wrote:
> >
> > > FWIW working on a codebase that heavily uses both Error and threads, the side-table seems fairly a bit terrifying. Even if the thread safety issues are fixed, do we really want to pay for mutex lock/unlock to create errors, to save a pointer?
> > >
> > > thread_local might be an option, though as I understand llvm/Support may have to be portable to environments where that doesn't work well.
> >
> >
> > An atomic pointer would work too. This cost is only paid on failure values and these are already heap allocated, so I'm I wouldn't expect the overhead to be prohibitive either way.
>
>
> I don't follow this - are you saying if heap allocation is OK, locking a mutex must be too, as it's cheaper?
>  Locking/unlocking a shared global mutex can certainly be more expensive than malloc if there's a lot of contention. malloc implementations typically go to some lengths to avoid a global lock for this reason.
>
> Example workload: clangd typically runs ~30 threads for background indexing, each thread frequently calls functions returning `Expected<T>` and recovers from errors (Error is documented as being for recoverable errors). It'd be a big surprise if this sort of use of Error led to contention/serialization on these worker threads.


Note this feature's only intended to be opt-in for debugging purposes, so for production use cases it shouldn't be a huge deal (though could be cheaper/free if it was a build-time setting rather than something you could opt into/out of at runtime - perhaps that's the point you're making? Though we do check various globals (cl::opts, etc) in a fair number of codepaths, I think, such that adding one doesn't seem likely to be super problematic?). (as much as I agree with you that I'd rather not have a side table)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70600