Skip to content

Commit 569e553

Browse files
committed
Copy the part we need of registeredAutoloads instead of holding the lock longer
* Since multiple calls to getAutoloadConstants() could contend and getAutoloadConstants() is called on every not-already-loaded #require it seems best to avoid contention here.
1 parent a4a205b commit 569e553

File tree

2 files changed

+26
-21
lines changed

2 files changed

+26
-21
lines changed

src/main/java/org/truffleruby/language/RubyConstant.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222

2323
public class RubyConstant implements ObjectGraphNode {
2424

25+
public static final RubyConstant[] EMPTY_ARRAY = new RubyConstant[0];
26+
2527
private final DynamicObject declaringModule;
2628
private final String name;
2729
private final Object value;

src/main/java/org/truffleruby/language/loader/FeatureLoader.java

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -98,39 +98,42 @@ public void addAutoload(RubyConstant autoloadConstant) {
9898

9999
public List<RubyConstant> getAutoloadConstants(String expandedPath) {
100100
final String basename = basenameWithoutExtension(expandedPath);
101-
final Map<String, List<RubyConstant>> constantsMap;
101+
final Map<String, RubyConstant[]> constantsMapCopy;
102102

103103
registeredAutoloadsLock.lock();
104104
try {
105-
constantsMap = registeredAutoloads.get(basename);
105+
final Map<String, List<RubyConstant>> constantsMap = registeredAutoloads.get(basename);
106106
if (constantsMap == null || constantsMap.isEmpty()) {
107107
return null;
108108
}
109109

110-
final List<RubyConstant> constants = new ArrayList<>();
110+
// Deep-copy constantsMap so we can call findFeature() outside the lock
111+
constantsMapCopy = new LinkedHashMap<>();
111112
for (Map.Entry<String, List<RubyConstant>> entry : constantsMap.entrySet()) {
112-
// NOTE: this call might be expensive but it seems difficult to move it outside the lock.
113-
// Contention does not seem a big issue since only addAutoload/removeAutoload need to wait.
114-
// At least, findFeature() does not access registeredAutoloads or use registeredAutoloadsLock.
115-
final String expandedAutoloadPath = findFeature(entry.getKey());
116-
117-
if (expandedPath.equals(expandedAutoloadPath)) {
118-
for (RubyConstant constant : entry.getValue()) {
119-
// Do not autoload recursively from the #require call in GetConstantNode
120-
if (!constant.getAutoloadConstant().isAutoloading()) {
121-
constants.add(constant);
122-
}
113+
constantsMapCopy.put(entry.getKey(), entry.getValue().toArray(RubyConstant.EMPTY_ARRAY));
114+
}
115+
} finally {
116+
registeredAutoloadsLock.unlock();
117+
}
118+
119+
final List<RubyConstant> constants = new ArrayList<>();
120+
for (Map.Entry<String, RubyConstant[]> entry : constantsMapCopy.entrySet()) {
121+
final String expandedAutoloadPath = findFeature(entry.getKey());
122+
123+
if (expandedPath.equals(expandedAutoloadPath)) {
124+
for (RubyConstant constant : entry.getValue()) {
125+
// Do not autoload recursively from the #require call in GetConstantNode
126+
if (!constant.getAutoloadConstant().isAutoloading()) {
127+
constants.add(constant);
123128
}
124129
}
125130
}
131+
}
126132

127-
if (constants.isEmpty()) {
128-
return null;
129-
} else {
130-
return constants;
131-
}
132-
} finally {
133-
registeredAutoloadsLock.unlock();
133+
if (constants.isEmpty()) {
134+
return null;
135+
} else {
136+
return constants;
134137
}
135138
}
136139

0 commit comments

Comments
 (0)