Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[refactor] Make taichi/common self contained #2200

Merged
merged 6 commits into from
Mar 7, 2021

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Mar 3, 2021

Related issue = #2196

The goal of this PR is to make taichi/common self-contained and not depend on any other part of the codebase. There are a few problems before:

  1. Logger's impl used to be in taichi/util, this creates a circular include between taichi/common and taichi/util.

logging.cpp gets moved to taichi/common.

  1. Logger itself is coupled with both signal handlers and pybind11.

Create a HackedSignalHandler, which takes care of these procedures in its constructor. We then instantiate a global HackedSignalHandler instead. The name "hacked" is mostly because I'm not sure if this is the best practice to invoke some initialization code in a shared library. There is a __attribute((constructor)) attribute, but it's a GCC specific extension..

  1. Remove the global logger, and switches to the singleton Logger::get_instance(). This is because global variables in different translation units are unspecified.

[Click here for the format server]


@k-ye k-ye requested a review from taichi-gardener March 4, 2021 13:20
@k-ye k-ye force-pushed the refactor-logging branch from e02246a to f779f62 Compare March 4, 2021 13:22
@k-ye k-ye changed the title [refactor] move logging to taichi/common [refactor] Makes taichi/common self contained Mar 4, 2021
@k-ye k-ye changed the title [refactor] Makes taichi/common self contained [refactor] Make taichi/common self contained Mar 4, 2021
@k-ye k-ye force-pushed the refactor-logging branch from e6b0fb7 to 4add00a Compare March 4, 2021 13:49
@k-ye k-ye requested review from yuanming-hu and xumingkuan March 4, 2021 22:12
Copy link
Member Author

@k-ye k-ye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the large change. This is mostly shuffling code around.

taichi/common/core.h Show resolved Hide resolved
taichi/common/core.h Show resolved Hide resolved
taichi/common/interface.h Show resolved Hide resolved
taichi/common/interfaces.cpp Show resolved Hide resolved
taichi/common/logging.cpp Outdated Show resolved Hide resolved
taichi/common/logging.cpp Show resolved Hide resolved
taichi/common/logging.h Show resolved Hide resolved
cmake/TaichiCore.cmake Show resolved Hide resolved
Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

Btw, do you expect the compilation/linking time to also improve after this series of refactoring, given we re-compile fewer headers? If that is the case, does it makes sense to set up a quantitative build time benchmark? (so that we can tell a more precise and quantitative story to future developers? Something like "after Ye's refactoring not only the code is more readable but also the compilation time is improved by 20%" :-)

cmake/TaichiCore.cmake Show resolved Hide resolved
taichi/common/interface.h Show resolved Hide resolved
taichi/common/logging.cpp Outdated Show resolved Hide resolved
taichi/common/logging.cpp Outdated Show resolved Hide resolved
taichi/common/logging.cpp Show resolved Hide resolved
Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more trap: the current build.py file only copies the libtaichi_core.pyd to the package. Before we release it may need fixing:

taichi/python/build.py

Lines 60 to 67 in dee35f4

if get_os_name() == 'linux':
shutil.copy('../build/libtaichi_core.so', 'taichi/lib/taichi_core.so')
elif get_os_name() == 'osx':
shutil.copy('../build/libtaichi_core.dylib',
'taichi/lib/taichi_core.so')
else:
shutil.copy('../runtimes/RelWithDebInfo/taichi_core.dll',
'taichi/lib/taichi_core.pyd')

Copy link
Contributor

@xumingkuan xumingkuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

taichi/common/logging.cpp Show resolved Hide resolved
@@ -0,0 +1,6 @@
#include "taichi/common/core.h"

namespace taichi {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we write namespace taichi as-is or use this one?

#define TI_NAMESPACE_BEGIN namespace taichi {

Copy link
Member Author

@k-ye k-ye Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think both are fine. At some point we should do a batch change from TI_NAMESPACE_BEGIN to namespace taichi { anyway.. Do you have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I also prefer namespace taichi {... It took me one more click in the IDE to know what TI_NAMESPACE_BEGIN was.

Co-authored-by: Yuanming Hu <yuanming-hu@users.noreply.github.com>
@k-ye
Copy link
Member Author

k-ye commented Mar 6, 2021

Looks great to me!

Btw, do you expect the compilation/linking time to also improve after this series of refactoring, given we re-compile fewer headers? If that is the case, does it makes sense to set up a quantitative build time benchmark? (so that we can tell a more precise and quantitative story to future developers? Something like "after Ye's refactoring not only the code is more readable but also the compilation time is improved by 20%" :-)

LOL, TBH I haven't really looked to much into the compilation duration problem in Taichi. To me it was slow but not unbearable. I think it's great to have a build perf benchmark, but maybe not that urgent? Also we should confirm whether the slowness comes from compilation (parallelizable) or linking (single-threaded), or both.. If the reprocessing of the headers is a major source of slowness, maybe we can try things like https://clang.llvm.org/docs/PCHInternals.html#using-precompiled-headers-with-clang. Anyhow, compilation optimization itself is a huge topic... :)

@k-ye k-ye force-pushed the refactor-logging branch from 03311b3 to 2b2672f Compare March 6, 2021 12:47
@k-ye
Copy link
Member Author

k-ye commented Mar 6, 2021

One more trap: the current build.py file only copies the libtaichi_core.pyd to the package. Before we release it may need fixing:

taichi/python/build.py

Lines 60 to 67 in dee35f4

if get_os_name() == 'linux':
shutil.copy('../build/libtaichi_core.so', 'taichi/lib/taichi_core.so')
elif get_os_name() == 'osx':
shutil.copy('../build/libtaichi_core.dylib',
'taichi/lib/taichi_core.so')
else:
shutil.copy('../runtimes/RelWithDebInfo/taichi_core.dll',
'taichi/lib/taichi_core.pyd')

Good point! But note that the newly added lib is what CMake calls an OBJECT library. IIUC, this is not a real static/shared library, but just a logical concept. Under the hood, it's just a bunch of .o files, which can then be included into other targets. See e.g. https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries and https://gitlab.kitware.com/cmake/community/-/wikis/doc/tutorials/Object-Library

@yuanming-hu
Copy link
Member

Looks great to me!
Btw, do you expect the compilation/linking time to also improve after this series of refactoring, given we re-compile fewer headers? If that is the case, does it makes sense to set up a quantitative build time benchmark? (so that we can tell a more precise and quantitative story to future developers? Something like "after Ye's refactoring not only the code is more readable but also the compilation time is improved by 20%" :-)

LOL, TBH I haven't really looked to much into the compilation duration problem in Taichi. To me it was slow but not unbearable. I think it's great to have a build perf benchmark, but maybe not that urgent? Also we should confirm whether the slowness comes from compilation (parallelizable) or linking (single-threaded), or both.. If the reprocessing of the headers is a major source of slowness, maybe we can try things like https://clang.llvm.org/docs/PCHInternals.html#using-precompiled-headers-with-clang. Anyhow, compilation optimization itself is a huge topic... :)

Sounds good. Improving compilation speed always makes development more enjoyable and make CI cost less but I agree with you that it is not urgent.

One more trap: the current build.py file only copies the libtaichi_core.pyd to the package. Before we release it may need fixing:

taichi/python/build.py

Lines 60 to 67 in dee35f4

if get_os_name() == 'linux':
shutil.copy('../build/libtaichi_core.so', 'taichi/lib/taichi_core.so')
elif get_os_name() == 'osx':
shutil.copy('../build/libtaichi_core.dylib',
'taichi/lib/taichi_core.so')
else:
shutil.copy('../runtimes/RelWithDebInfo/taichi_core.dll',
'taichi/lib/taichi_core.pyd')

Good point! But note that the newly added lib is what CMake calls an OBJECT library. IIUC, this is not a real static/shared library, but just a logical concept. Under the hood, it's just a bunch of .o files, which can then be included into other targets. See e.g. https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries and https://gitlab.kitware.com/cmake/community/-/wikis/doc/tutorials/Object-Library

I see! I misunderstood. I thought an extra shared obj was created.

@k-ye k-ye merged commit 064971d into taichi-dev:master Mar 7, 2021
@k-ye k-ye deleted the refactor-logging branch March 7, 2021 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants