GitLab

Created by: mhoemmen

@trilinos/tpetra Blocks: #941 (closed)

CrsMatrix::getCrsGraph returns a Teuchos::RCP of the matrix's graph. This is not currently thread safe, because RCP's reference count is not currently thread safe. In particular, RCP's copy constructor, assignment operator (operator=), and destructor all modify the reference count, so none of these are thread safe. The getCrsGraph method must increment the graph's reference count, since it returns the RCP by value. Thus, it cannot be thread safe. Returning const RCP& is not safe (threads or no), because callers can access it even after the lifetime of the graph has ended.

Today, I was reminded of this lack of thread safety the hard way, while host-thread-parallelizing CrsMatrix::mergeRedundantValues (see #941 (closed)). Many methods in CrsMatrix look "harmless" -- they appear just to access some local state like the number of rows owned by the calling process -- but are not thread safe. This is because they call getCrsGraph, then ask the graph for information. Many of the methods that ask the graph for information are themselves thread safe, but the getCrsGraph call itself is the problem. I was able to fix the thread safety issue in my changes to mergeRedundantValues, by lifting the relevant call (isStorageOptimized()) outside of the host-thread-parallel loop. However, I would like to forestall this issue for future work.

I propose the following fix: add a new private method, getCrsGraphRef, to CrsMatrix. The new method will return const crs_graph_type&. Its implementation must carefully avoid any RCP methods that would change the RCP's reference count.