Page MenuHomeSoftware Heritage

Add a flyweight copy() to SwhGraphProperties to make it threadsafe
AcceptedPublic

Authored by seirl on Aug 4 2022, 6:06 PM.

Details

Reviewers
JaredR26
Group Reviewers
Reviewers
Maniphest Tasks
T4422: Graph property access is not thread-safe
Summary

See T4422 for details.

Diff Detail

Event Timeline

Build is green

Patch application report for D8191 (id=29562)

Rebasing onto 555e3b6d3e...

Current branch diff-target is up to date.
Changes applied before test
commit 66e94bc43d9e3ba4323b71166082c04672bc1388
Author: Antoine Pietri <antoine.pietri1@gmail.com>
Date:   Thu Aug 4 18:06:10 2022 +0200

    Add a flyweight copy() to SwhGraphProperties to make it threadsafe

See https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/214/ for more details.

seirl requested review of this revision.Aug 4 2022, 6:12 PM
JaredR26 added a subscriber: JaredR26.
JaredR26 added inline comments.
java/src/main/java/org/softwareheritage/graph/SwhGraphProperties.java
80–84

FYI, timestamps and content lengths were already threadsafe for reading for me, or if it wasn't, it didn't cause any issues I found. edgeLabelNames and nodeIdMap were the problems, and I'm guessing any of the other string ones.

java/src/main/java/org/softwareheritage/graph/SwhUnidirectionalGraph.java
152–153

I'm not sure it matters, but because this.graph can refer to the underlying graph of this.labelledGraph (Line 71, inside loadLabelledGraphOnly), that would result in double-copying. (When you first wrote the copy function, we hadn't yet found the need to assign the underlying to graph) I'm sure it is low overhead, but would it be better to first copy this.labelledGraph and then assign g.g again if labelledGraph exists?

java/src/main/java/org/softwareheritage/graph/maps/NodeTypesMap.java
57–61

FYI, this was already threadsafe for reading for me.

This revision is now accepted and ready to land.Aug 4 2022, 6:52 PM