[core] Simplify ErrorHandler implementation #19395
Conversation
The current implementation uses a stack-allocated buffer + an optionally- allocated thread local heap buffer for vsnprintf. This likely was made to avoid heap allocating whenever not necessary (i.e. in the majority of cases). However, a subsequent commit introduced a string `bp` that always performs the copy anyway. At this point the code can be simplified to avoid the double buffer and simply allocate straight into the string avoiding an extra copy.
silverweed
force-pushed
the
error_handler
branch
from
7b3a187 to
d0b0494
Compare
yes, this is the case (In particular this is useful because of the not so rare case where the ErrorHandler ends up being called during signal handling).
humm :( That is actually bad. The original PR #5855, the reviewers completely missed this unexpected but significant change (the commit is "remove gEnv and gSystem deps from TError " ...). We should revert the part of the commit that introduce the unwanted memory allocation rather than doubling down on it ... (or alternatively re-implement the function so that it returns in another way to having no memory allocation in most cases). |
Can you explain why you think this is critical? After all, we are talking about an error handler, so by definition this should not be on the critical path. |
|
@hahnjo unfortunately the ErrorHandler is also called on |
|
... which wasn't a problem for five years... |
|
Nobody complained about it for five years, which is a weaker statement. But I am also not convinced it's something we actually need to address... |
|
Ping @pcanal |
The current implementation uses a stack-allocated buffer + an optionally- allocated thread local heap buffer for vsnprintf. This likely was made to avoid heap allocating whenever not necessary (i.e. in the majority of cases).
However, a subsequent commit introduced a string
bpthat always performs the copy anyway. At this point the code can be simplified to avoid the double buffer and simply allocate straight into the string avoiding an extra copy.