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

Valgrind is no longer supported in 5.0 but no reason was provided #910

Closed
thedrow opened this Issue Jun 15, 2017 · 3 comments

Comments

Projects
None yet
3 participants
@thedrow
Copy link

thedrow commented Jun 15, 2017

Does this mean we cannot debug memory leaks with Valgrind anymore? Is the support no longer needed and Valgrind just works?
What are the implications here? They should be described in the release notes if possible.

@interwq

This comment has been minimized.

Copy link
Member

interwq commented Jun 15, 2017

Please see the discussion in #369.

@interwq interwq added the question label Jun 15, 2017

@thedrow

This comment has been minimized.

Copy link
Author

thedrow commented Jun 16, 2017

At the very least, the release notes should include a reference to that issue.
This has serious implications towards ease of development with jemalloc.
Essentially you guys are saying that it is no longer possible to detect memory leaks with jemalloc easily which will be a blocker for a lot of projects. Especially large projects that utilize jemalloc's API instead of just using a better malloc.

@jasone

This comment has been minimized.

Copy link
Member

jasone commented Jun 16, 2017

@thedrow, we make no such claim regarding lack of memory leak diagnostics capabilities. In fact, the statistical heap profiling functionality that has been part of jemalloc since 1.0.0 tends to be more versatile than Valgrind for the purposes of leak detection and diagnosis, because it imposes little enough overhead to be usable in all but the most performance-sensitive use cases.

Regarding the ChangeLog, our goal is to succinctly enumerate all the changes that have potential relevance to people as they adopt new versions, as an aid in deciding what needs further investigation. The ChangeLog is not intended as a detailed rationale for the entire commit history. The full git history is available for your perusal, and you can look at the 5.0.0 milestone issues for a different perspective.

Stepping back, I get the sense you're really upset about the feature removal, rather than how its removal was documented. We had Valgrind integration support for all of the 3.x and 4.x releases, but the feature imposed substantial internal complexity (redzones and quarantine), and regressions often went unreported for multiple releases, especially once my colleagues at Facebook shifted their memory correctness testing from Valgrind to ASan. The integration was notoriously easy to subtly break, and we had no Valgrind-specific regression tests. When I considered how poorly we were maintaining Valgrind integration, in combination with its reduced role relative to ASan, and the desire to add features like arena reset/destroy that Valgrind integration would have made much more difficult, the choice was clear. If there were active jemalloc developers who cared about maintaining high quality Valgrind integration, we might instead have chosen to improve our test coverage, but we didn't even have consistent regression reporting, let alone active interest. Given a choice of complaints due to feature removal, and the dissatisfaction of shipping faulty code for a diminishing use case, I choose the former.

@jasone jasone closed this Jun 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.