-
Notifications
You must be signed in to change notification settings - Fork 61
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
V1.1 branch #243
V1.1 branch #243
Conversation
…nto a very old version of GraphBLAS)
I have a few more changes to add to this PR, based on feedback from the SuiteSparse 7.4.0.beta1 release. I'm testing them now and will add them to this PR shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, Lots of missing frees! @DrTimothyAldenDavis the Boehm garbage collection library has a leak detection maybe it makes sense to try and interface that (maybe at the SS level) as another sanity check.
The src/algorithms all have "brutal" tests, which have a "brutal malloc". The brutal malloc is given a count: you have (say) 5 mallocs. Count down to zero, and then after that always return NULL. Then in the brutal tests, I give the algorithm 0 mallocs, and check the result, then 1, then 2, etc, until it succeeds. The brutal malloc also counts malloc's vs free's, and then the test fails if the mallocs minus frees is not zero. The LAGraph_Free also passes the size of the block being freed, so if the count of bytes still malloc'ed is not zero, then the test fails. This 'brutal' process is thus able to detect leaks that occur when running out of memory. That is, the method should check if it gets a NULL from malloc, and if it fails that way it should free any temporary workspace, or the result if that's been allocated. So, there were no leaks in the src/utility or src/algorithms. Not even any leaks in the src/tests. Still, the brutal tests could simultaneously use a valgrind option, or other checker. The experimental algorithms, utilities, and tests typically don't have brutal tests. I had a segfault so I ran all the tests, including experimental, using valgrind, and that's when I found all those leaks. So I patched them. I think that now both the src and experimental algos, utilities, and tests are all leak free. Still, a leak checker would be nice, particularly if it works on Windows and Mac (valgrind doesn't work on Windows, I think. Nor on Mac). |
Regarding the brutal memory tests: I could also add my own memory table for debugging only. I do this in GraphBLAS: each malloc gets logged in its table, with its size. A free passes in the size, too (not just the pointer) and the size much match exactly or an error is thrown. Then I can also check if the pointer passed to free is valid (it must be in the table). Then when GrB_Finalize is called, it checks to ensure the table is empty, and throws an error ("leak!") if it's not empty. It would not be hard to add this to LAGraph too, since all LAGraph_malloc's go through the same malloc function that GraphbLAS uses. |
Also: the *dev2 branches are not tested with the CI because they sometimes depend on GraphBLAS versions that are in the CI yet. Some branches (v1.2_branch in particular) requires the GrB get/set, which is in the draft SuiteSparse:GraphBLAS 9.0.x. The build.yaml file only tests with GraphBLAS 7.x, so no JIT, no GrB_get/set, and so on. Once the v2.1 C API becomes stable, and GraphBLAS 8.2.x, 8.3.x, and 9.0.x can be added to the build.yml file, then it would be possible to enable the CI on more branches. |
Co-authored-by: Markus Mützel <[email protected]>
…to v1.1_branch
Dec 30, 2023: version 1.1.0