search

GraphBLAS / LAGraph

This is a library plus a test harness for collecting algorithms that use the GraphBLAS. For test coverage reports, see https://graphblas.org/LAGraph/ . Documentation: https://lagraph.readthedocs.org
Other
225 stars 59 forks source link

V1.1 branch #243

Closed DrTimothyAldenDavis closed 8 months ago

DrTimothyAldenDavis commented 8 months ago

Dec 30, 2023: version 1.1.0

* major change to build system: by Markus Mützel
* port: to 32-bit systems
DrTimothyAldenDavis commented 8 months ago

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.

DrTimothyAldenDavis commented 8 months ago

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).

DrTimothyAldenDavis commented 8 months ago

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.

DrTimothyAldenDavis commented 8 months ago

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.