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

V1.1 branch #243

Merged
merged 43 commits into from
Dec 30, 2023
Merged

V1.1 branch #243

merged 43 commits into from
Dec 30, 2023

Conversation

DrTimothyAldenDavis
Copy link
Member

Dec 30, 2023: version 1.1.0

* major change to build system: by Markus Mützel
* port: to 32-bit systems

@DrTimothyAldenDavis
Copy link
Member Author

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.

Copy link
Member

@michelp michelp left a 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.

.github/workflows/build.yml Show resolved Hide resolved
ChangeLog Outdated Show resolved Hide resolved
src/test/test_minmax.c Show resolved Hide resolved
@DrTimothyAldenDavis
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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.

config/LAGraph.pc.in Outdated Show resolved Hide resolved
Co-authored-by: Markus Mützel <[email protected]>
@DrTimothyAldenDavis DrTimothyAldenDavis merged commit c6dcf33 into stable Dec 30, 2023
12 checks passed
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.

3 participants