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

Two fixes for zero allocation logging #217

Closed
wants to merge 1 commit into from
Closed

Conversation

shinh
Copy link
Collaborator

@shinh shinh commented Jul 26, 2017

  • Do not use non-POD type for TLS. This is for Issue MSVC 2013 build has been broken #213.
  • Use GNU's __thread if it's available. On my linux box, cmake
    wrongly thinks attribute((thread)), which is for Cygwin,
    is available.

- Do not use non-POD type for TLS. This is for Issue google#213.
- Use GNU's __thread if it's available. On my linux box, cmake
  wrongly thinks __attribute__((thread)), which is for Cygwin,
  is available.
@sergiud
Copy link
Collaborator

sergiud commented Jul 26, 2017

Your PR seems to leak memory. I have a working patch for the related issues which does not allocate dynamic memory. I however need to fix the tests.

@shinh
Copy link
Collaborator Author

shinh commented Jul 26, 2017

IMO not freeing heap-allocated memory is fine as long as it can be accessible via a global pointer. Tools like valgrind should categorize this as "still reachable" not a leak. I agree it'd be nicer we can get rid of it though. I'm looking forward to your patch, thanks!

$ cat still_reachable.cc
__thread int* global;
int main() {
global = new int();
}
$ g++ still_reachable.cc
$ valgrind --leak-check=full ./a.out
...
==137829== HEAP SUMMARY:
==137829== in use at exit: 4 bytes in 1 blocks
==137829== total heap usage: 1 allocs, 0 frees, 4 bytes allocated
==137829==
==137829== LEAK SUMMARY:
==137829== definitely lost: 0 bytes in 0 blocks
==137829== indirectly lost: 0 bytes in 0 blocks
==137829== possibly lost: 0 bytes in 0 blocks
==137829== still reachable: 4 bytes in 1 blocks
==137829== suppressed: 0 bytes in 0 blocks

@DariuszOstolski
Copy link
Contributor

What if your glog client application create and destroy 1000 threads (because it is supposed to work continuously for 2 years and is creating 3 threads per day for various tasks) ? At some point of time your pull request will bring a trouble(sooner or later) and not freeing heap allocated memory in C++ is real issue for every application.

@sergiud sergiud mentioned this pull request Aug 8, 2017
@sergiud
Copy link
Collaborator

sergiud commented Aug 8, 2017

Added #226.

@shinh shinh closed this Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants