Enable specifying stacktrace size at runtime. #470
Conversation
trivialfis
force-pushed
the
stacktrace
branch
3 times, most recently
from
248d038 to
fcf38a3
Compare
trivialfis
changed the title
|
Still trying to figure out the best way to profile the memory usage. :) Sorry for these early PRs. |
|
@trivialfis Don't be sorry. Your contribution to dmlc-core is quite valuable. |
* Add extra optional parameter to StackTrace. * Enable finding GTest installed in system * Make build_config.h for unittests private.
trivialfis
force-pushed
the
stacktrace
branch
from
fcf38a3 to
4015880
Compare
trivialfis
changed the title
|
@trivialfis Is this PR ready for review? |
|
@hcho3 Yes. Along with dmlc/xgboost#3795 . The only problem is we are hesitating to merge it due to:
Merging dmlc/xgboost#3795 will be another large chunk of code, we are not sure it's worthy. You can take a look into it if you have the time. |
|
@trivialfis Looking at this PR in isolation, I think the ability to specify stacktrace size at runtime would be useful to other people as well. Let us evaluate the merits of dmlc/xgboost#3795 at later time, after we merge this change. |
|
@hcho3 Thanks. It's ready for review. |
trivialfis
force-pushed
the
stacktrace
branch
from
e61eba2 to
7b58ad8
Compare
* Make StackTrace size unsigned. * Don't output build_config.h into binary directory.
trivialfis
force-pushed
the
stacktrace
branch
from
7b58ad8 to
80527a0
Compare
|
@trivialfis On a related note, let's add an option to XGBoost to enable "rich" backtraces: https://github.com/bombela/backward-cpp. Right now it only supports UNIX-like systems, but we can add it as an optional feature in the developer guide (think sanitizers). |
|
@hcho3 Seems interesting. From my experience, I can barely get the backtrace working in dmlc-core and failed to make a reliable test for it. I will take a look into that at the weekend. |
|
@hcho3 I looked into the source code of backward-cpp. It supports a wide ranges of "backend" libraries. It's already very messy to achieve what it does, if we were to add this into dmlc-core, I think we better limit the support to For build option, we can utilize CMake's But let me focus on dmlc/xgboost#3825 first, I'm a little bit tied up to that currently. I will come back to this. |
|
@trivialfis I don't think we should add backward-cpp to dmlc-core, since it is quite messy. It should be added to xgboost directly. |
|
@trivialfis Actually, I take back what I said, since all errors get directed to Anyway, let's hold it for now, since we have other things to worry about. Also, I think we want to "modernize" the CMake build before we attempt to do something fancy like backward-cpp. See discussion at #460 about Modern CMake practices. |
* Enable specifying trace size at runtime. * Add extra optional parameter to StackTrace. * Enable finding GTest installed in system * Make build_config.h for unittests private. * Address reviewer's comment. * Make StackTrace size unsigned. * Don't output build_config.h into binary directory.
@hcho3
This PR also groups GTest CMake configuration together. Seems to have some conflicts with previous commits. Let me see if it breaks the tests.
The change is mostly for dmlc/xgboost#3795 . With this I don't need the
PROFILINGoption.