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

Reorganize sources #85

Merged
merged 3 commits into from
Jun 5, 2020
Merged

Reorganize sources #85

merged 3 commits into from
Jun 5, 2020

Conversation

Flamefire
Copy link
Contributor

This does the discussed reorganization in preparation for cTracing.

File structure:

  • src/. contains glue code to define the extension module
  • src/scorepy contains namespaced implementation

Introduced optimization barries by using multiple cpp files can be removed by using LinkTimeOptimization and (optionally) visibility.

As it is related I also included the renaming if instrumenters to the "private" (by convention) name "_instrumenters". This signals that it is internally used only and allows developers to make breaking changes in that sub-package as long as externally visible behavior remains the same.

File structure:
  - src/*.* contains glue code to define the extension module
  - src/scorepy contains namespaced implementation

Introduced optimization barries by using multiple cpp files can be
removed by using LinkTimeOptimization and (optionally) visibility.
@Flamefire
Copy link
Contributor Author

@AndreasGocht Travis shows up 3 times but there is only 1 real build (which passed). Guess a temporary issue on GH/travis

@AndreasGocht
Copy link
Collaborator

The Travis CI test does complete. So it looks like an issue with the Travis GitHub connection.

@AndreasGocht
Copy link
Collaborator

Introduced optimization barries by using multiple cpp files can be removed by using LinkTimeOptimization and (optionally) visibility.

Can you elaborate this in a bit more detail?

@Flamefire
Copy link
Contributor Author

Can you elaborate this in a bit more detail?

In the call I mentioned that prior to this PR the C binding function static PyObject* parameter_int(... calls scorepy::parameter_int(name, value); which could be inlined into the former if the compiler chooses so saving 1 call and potentially allowing some more optimizations. Now that the scorepy:: function is in a separate TU (translation unit) the compiler can't see the definition and hence can't inline it which may introduce some overhead which wasn't there before. However this is at the C-level so that is dwarfed by the overhead of the instrumentation alone and simple calls are quite cheap in C.

There are 2 ways to improve:

  • Enabling LTO allows the compiler to optimize the created library as-if it was a single TU. So inlining can happen at link time and all is the same as before
  • Related is to compile with -fvisibility=hidden, which will reduce the binary size by removing most exported symbols. Remember that on Unix every single function, class, etc. is exported from a shared library so it could be used by another binary importing the shared lib. So all (mangled and hence long) names are contained making the library large. Using hidden visibility reverses this and nothing is exported by default (you have to export things explicitly). This reduces binary size and may enable better optimizations, especially LTO: If the compiler knows, this function is not used outside the library it is more likely to inline it into the only usage site. With default visibility even when inlining it there has to be an out-of-line definition for other libraries to call.

Hope this clears it up.
IMO this is a minor optimizations so we can also talk about and implement this later if a call is required.

@AndreasGocht
Copy link
Collaborator

Now I got it 😄 . Thanks.
I would propose, that we do a benchmark of the compiler settings, once the new instrumenter is there. So we could measure if the performance benefit is worth the additional complexity.

@AndreasGocht AndreasGocht merged commit a59883b into score-p:master Jun 5, 2020
@Flamefire Flamefire deleted the renaming branch June 5, 2020 09:43
@Flamefire
Copy link
Contributor Author

Flamefire commented Jun 5, 2020

Added issue #91 to not forget that

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.

2 participants