[Logging] Add DMLC_NO_INLINE to LogMessageFatal
#615
Conversation
junrushao
force-pushed
the
feature/2020-05-20/stack-usage-strikes-back
branch
from
37c77e6
to
6d84163
Compare
| return dmlc::Error(log_stream.str()); | ||
| } | ||
| DMLC_NO_INLINE static Entry *ThreadLocal() { | ||
| static thread_local Entry *result = new Entry(); |
There was a problem hiding this comment.
The introduction of TLS helps to reduce the size of class dmlc::LogMessageFatal to zero, which enables some opportunities in compiler optimization
|
@trivialfis Can you review? |
| std::ostringstream &stream() { return log_stream_; } | ||
| ~LogMessageFatal() DMLC_THROW_EXCEPTION { | ||
| std::ostringstream &stream() { return Entry::ThreadLocal()->log_stream; } | ||
| DMLC_NO_INLINE ~LogMessageFatal() DMLC_THROW_EXCEPTION { |
There was a problem hiding this comment.
The no-inline trick here instructs the compiler not to merge the complicated logic of throwing in the destructor into the caller. This usually wouldn't hurt performance because it is quite a slow path
| #endif | ||
| throw Error(log_stream_.str()); | ||
| throw Entry::ThreadLocal()->Finalize(); |
There was a problem hiding this comment.
We moved all the logic except throw itself to another class, to make sure the unsafe region is minimized
|
|
|
@junrushao1994 Somehow this line is not being honored: |
|
Merging this for now. We should also migrate the windows test to the action later. |
|
We can refer to the windows pipeline of TVM |
|
Hmmm i am not so sure why it happens. Is there any symbol overriding that happens in |
|
I am not too sure either, we should look into the cmakefile to see what was happening. One potential reason could due to the cmake cache if there is a separate libdmlc built with a different flag |
|
Here is a guess, it could be possible that we links every test into a single app. then the config in https://github.com/dmlc/dmlc-core/blob/master/test/unittest/unittest_logging.cc conflicts with the fatal throw one. Perhaps we should just unify the two tests if that is the case |
|
@junrushao1994 can you open a followup PR to do that? |
|
@tqchen to unify those two tests by changing both of their macro to 1? |
|
Yes, you might also need to update the ASSERT_DEATH a bit by changing that to throw |
A follow-up PR of #613
Below are the benchmarks we did. We use
g++ -fstack-usageto dump the stack usage information for the target function.Settings:
virtual tvm::relay::Expr tvm::relay::CommonSubexprEliminator::VisitExpr_(const tvm::relay::CallNode*), which originally crashed this PR on a very deep networkResults:
See also: apache/tvm#5585
CC: @tqchen @trivialfis