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
229 stars 61 forks source link

Make LAGraph_cc_fastsv5b thread-safe #93

Closed marci543 closed 4 years ago

marci543 commented 4 years ago

@zyz915 Can you please review this?

marci543 commented 4 years ago

I have a question regarding the sanitization. This part seems odd to me because it writes back the exact same A which was the input. https://github.com/GraphBLAS/LAGraph/blob/9e90a29224ebfaa680b9bb9f5972db99940e3dbc/Source/Algorithm/LAGraph_cc_fastsv5b.c#L503-L510 If I understand correctly, the function performs the sanitization on the original input and wants to return that. I think there is no need to pass a GrB_Matrix* for that, GrB_Matrix is enough. Creation of matrix S can also be saved if the eWiseAdd is done in place. https://github.com/GraphBLAS/LAGraph/blob/9e90a29224ebfaa680b9bb9f5972db99940e3dbc/Source/Algorithm/LAGraph_cc_fastsv5b.c#L259-L260

DrTimothyAldenDavis commented 4 years ago

It's because the function exports the user's input matrix, so it can get access to the internal CSR or CSC form. The export is destructive. It then reimports the matrix on return. So the content of the user's matrix is not modified, but the pointer to the matrix can change (the header is freed, then reallocated). It's less than ideal, of course. It would be better to be able to do this with passing in a GrB_Matrix A, not GrB_Matrix *A. But then import/export can't be used.

marci543 commented 4 years ago

I see. Thanks for the explanation. I added a note for that.

DrTimothyAldenDavis commented 4 years ago

Thanks -- the extra note is helpful. Ideally, a future version will avoid this issue. It wouldn't be suitable in a production version of LAGraph.