Skip to content

Commit f18b9df

Browse files
eregoneregon
committed
[GR-19220] Improve performance of class variables (#2259)
PullRequest: truffleruby/2424
2 parents 040cbb1 + c52da5a commit f18b9df

17 files changed

+428
-111
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ Performance:
6161
* Improve the performance of checks for recursion (#2189, @LillianZ).
6262
* Improve random number generation performance by avoiding synchronization (#2190, @ivoanjo).
6363
* We now create a single call target per block by default instead of two.
64+
* Some uses of class variables are now much better optimized (#2259, @chrisseaton).
6465

6566
Changes:
6667

‎src/main/java/org/truffleruby/RubyLanguage.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@
8282
import org.truffleruby.language.RubyInlineParsingRequestNode;
8383
import org.truffleruby.language.RubyParsingRequestNode;
8484
import org.truffleruby.language.objects.RubyObjectType;
85+
import org.truffleruby.language.objects.classvariables.ClassVariableStorage;
8586
import org.truffleruby.options.LanguageOptions;
8687
import org.truffleruby.platform.Platform;
8788
import org.truffleruby.shared.Metrics;
@@ -211,6 +212,12 @@ public final class RubyLanguage extends TruffleLanguage<RubyContext> {
211212
public final Shape unboundMethodShape = createShape(RubyUnboundMethod.class);
212213
public final Shape weakMapShape = createShape(RubyWeakMap.class);
213214

215+
public final Shape classVariableShape = Shape
216+
.newBuilder()
217+
.allowImplicitCastIntToLong(true)
218+
.layout(ClassVariableStorage.class)
219+
.build();
220+
214221
public RubyLanguage() {
215222
coreMethodAssumptions = new CoreMethodAssumptions(this);
216223
coreStrings = new CoreStrings(this);

‎src/main/java/org/truffleruby/core/module/ModuleFields.java

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.concurrent.ConcurrentMap;
2222
import java.util.concurrent.locks.ReentrantLock;
2323

24+
import com.oracle.truffle.api.object.DynamicObjectLibrary;
2425
import org.jcodings.specific.UTF8Encoding;
2526
import org.truffleruby.RubyContext;
2627
import org.truffleruby.RubyLanguage;
@@ -46,6 +47,7 @@
4647
import org.truffleruby.language.methods.InternalMethod;
4748
import org.truffleruby.language.objects.ObjectGraph;
4849
import org.truffleruby.language.objects.ObjectGraphNode;
50+
import org.truffleruby.language.objects.classvariables.ClassVariableStorage;
4951
import org.truffleruby.language.objects.shared.SharedObjects;
5052

5153
import com.oracle.truffle.api.Assumption;
@@ -95,7 +97,7 @@ public static void debugModuleChain(RubyModule module) {
9597

9698
private final ConcurrentMap<String, InternalMethod> methods = new ConcurrentHashMap<>();
9799
private final ConcurrentMap<String, RubyConstant> constants = new ConcurrentHashMap<>();
98-
private final ConcurrentMap<String, Object> classVariables = new ConcurrentHashMap<>();
100+
private final ClassVariableStorage classVariables;
99101

100102
/** The refinements (calls to Module#refine) nested under/contained in this namespace module (M). Represented as a
101103
* map of refined classes and modules (C) to refinement modules (R). */
@@ -121,6 +123,7 @@ public ModuleFields(
121123
this.rubyModule = rubyModule;
122124
this.methodsUnmodifiedAssumption = new CyclicAssumption("methods are unmodified");
123125
this.constantsUnmodifiedAssumption = new CyclicAssumption("constants are unmodified");
126+
classVariables = new ClassVariableStorage(language);
124127
start = new PrependMarker(this);
125128
}
126129

@@ -193,7 +196,12 @@ public void initCopy(RubyModule from) {
193196
this.constants.put(entry.getKey(), entry.getValue());
194197
}
195198

196-
this.classVariables.putAll(fromFields.classVariables);
199+
for (Object key : fromFields.classVariables.getShape().getKeys()) {
200+
final Object value = fromFields.classVariables.read((String) key, DynamicObjectLibrary.getUncached());
201+
if (value != null) { // do not copy if it was removed concurrently
202+
this.classVariables.put((String) key, value, DynamicObjectLibrary.getUncached());
203+
}
204+
}
197205

198206
if (fromFields.hasPrependedModules()) {
199207
// Then the parent is the first in the prepend chain
@@ -745,7 +753,9 @@ public InternalMethod getMethod(String name) {
745753
return methods.get(name);
746754
}
747755

748-
public ConcurrentMap<String, Object> getClassVariables() {
756+
/** All write accesses to this object should use {@code synchronized (getClassVariables()) { ... }}, or check that
757+
* the ClassVariableStorage Shape is not shared */
758+
public ClassVariableStorage getClassVariables() {
749759
return classVariables;
750760
}
751761

@@ -849,8 +859,11 @@ public void getAdjacentObjects(Set<Object> adjacent) {
849859
ObjectGraph.addProperty(adjacent, constant);
850860
}
851861

852-
for (Object value : classVariables.values()) {
853-
ObjectGraph.addProperty(adjacent, value);
862+
for (Object key : classVariables.getShape().getKeys()) {
863+
final Object value = classVariables.read((String) key, DynamicObjectLibrary.getUncached());
864+
if (value != null) {
865+
ObjectGraph.addProperty(adjacent, value);
866+
}
854867
}
855868

856869
for (InternalMethod method : methods.values()) {

‎src/main/java/org/truffleruby/core/module/ModuleNodes.java

Lines changed: 35 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.util.concurrent.ConcurrentMap;
2020

2121
import com.oracle.truffle.api.library.CachedLibrary;
22+
import com.oracle.truffle.api.profiles.ConditionProfile;
2223
import org.jcodings.specific.UTF8Encoding;
2324
import org.truffleruby.RubyContext;
2425
import org.truffleruby.RubyLanguage;
@@ -60,7 +61,6 @@
6061
import org.truffleruby.core.string.StringUtils;
6162
import org.truffleruby.core.support.TypeNodes;
6263
import org.truffleruby.core.symbol.RubySymbol;
63-
import org.truffleruby.core.symbol.SymbolTable;
6464
import org.truffleruby.language.LexicalScope;
6565
import org.truffleruby.language.Nil;
6666
import org.truffleruby.language.NotProvided;
@@ -103,6 +103,10 @@
103103
import org.truffleruby.language.objects.ReadInstanceVariableNode;
104104
import org.truffleruby.language.objects.SingletonClassNode;
105105
import org.truffleruby.language.objects.WriteInstanceVariableNode;
106+
import org.truffleruby.language.objects.classvariables.CheckClassVariableNameNode;
107+
import org.truffleruby.language.objects.classvariables.ClassVariableStorage;
108+
import org.truffleruby.language.objects.classvariables.LookupClassVariableNode;
109+
import org.truffleruby.language.objects.classvariables.SetClassVariableNode;
106110
import org.truffleruby.language.yield.CallBlockNode;
107111
import org.truffleruby.parser.Identifiers;
108112
import org.truffleruby.parser.ParserContext;
@@ -804,14 +808,12 @@ protected RubyNode coerceToString(RubyNode name) {
804808
return NameToJavaStringNode.create(name);
805809
}
806810

807-
@TruffleBoundary
808811
@Specialization
809-
protected boolean isClassVariableDefinedString(RubyModule module, String name) {
810-
SymbolTable.checkClassVariableName(getContext(), name, module, this);
811-
812-
final Object value = ModuleOperations.lookupClassVariable(module, name);
813-
814-
return value != null;
812+
protected boolean isClassVariableDefinedString(RubyModule module, String name,
813+
@Cached CheckClassVariableNameNode checkClassVariableNameNode,
814+
@Cached LookupClassVariableNode lookupClassVariableNode) {
815+
checkClassVariableNameNode.execute(module, name);
816+
return lookupClassVariableNode.execute(module, name) != null;
815817
}
816818

817819
}
@@ -827,13 +829,14 @@ protected RubyNode coerceToString(RubyNode name) {
827829
}
828830

829831
@Specialization
830-
@TruffleBoundary
831-
protected Object getClassVariable(RubyModule module, String name) {
832-
SymbolTable.checkClassVariableName(getContext(), name, module, this);
833-
834-
final Object value = ModuleOperations.lookupClassVariable(module, name);
832+
protected Object getClassVariable(RubyModule module, String name,
833+
@Cached CheckClassVariableNameNode checkClassVariableNameNode,
834+
@Cached LookupClassVariableNode lookupClassVariableNode,
835+
@Cached ConditionProfile undefinedProfile) {
836+
checkClassVariableNameNode.execute(module, name);
837+
final Object value = lookupClassVariableNode.execute(module, name);
835838

836-
if (value == null) {
839+
if (undefinedProfile.profile(value == null)) {
837840
throw new RaiseException(
838841
getContext(),
839842
coreExceptions().nameErrorUninitializedClassVariable(module, name, this));
@@ -856,12 +859,11 @@ protected RubyNode coerceToString(RubyNode name) {
856859
}
857860

858861
@Specialization
859-
@TruffleBoundary
860-
protected Object setClassVariable(RubyModule module, String name, Object value) {
861-
SymbolTable.checkClassVariableName(getContext(), name, module, this);
862-
863-
ModuleOperations.setClassVariable(getLanguage(), getContext(), module, name, value, this);
864-
862+
protected Object setClassVariable(RubyModule module, String name, Object value,
863+
@Cached CheckClassVariableNameNode checkClassVariableNameNode,
864+
@Cached SetClassVariableNode setClassVariableNode) {
865+
checkClassVariableNameNode.execute(module, name);
866+
setClassVariableNode.execute(module, name, value);
865867
return value;
866868
}
867869

@@ -873,15 +875,17 @@ public abstract static class ClassVariablesNode extends CoreMethodArrayArguments
873875
@TruffleBoundary
874876
@Specialization
875877
protected RubyArray getClassVariables(RubyModule module) {
876-
final Map<String, Object> allClassVariables = ModuleOperations.getAllClassVariables(module);
877-
final int size = allClassVariables.size();
878-
final Object[] store = new Object[size];
878+
final Set<Object> variables = new HashSet<>();
879879

880-
int i = 0;
881-
for (String variable : allClassVariables.keySet()) {
882-
store[i++] = getSymbol(variable);
883-
}
884-
return createArray(store);
880+
ModuleOperations.classVariableLookup(module, m -> {
881+
final ClassVariableStorage classVariableStorage = m.fields.getClassVariables();
882+
for (Object key : classVariableStorage.getShape().getKeys()) {
883+
variables.add(getSymbol((String) key));
884+
}
885+
return null;
886+
});
887+
888+
return createArray(variables.toArray());
885889
}
886890
}
887891

@@ -1941,10 +1945,10 @@ protected RubyNode coerceToString(RubyNode name) {
19411945
return NameToJavaStringNode.create(name);
19421946
}
19431947

1944-
@TruffleBoundary
19451948
@Specialization
1946-
protected Object removeClassVariableString(RubyModule module, String name) {
1947-
SymbolTable.checkClassVariableName(getContext(), name, module, this);
1949+
protected Object removeClassVariableString(RubyModule module, String name,
1950+
@Cached CheckClassVariableNameNode checkClassVariableNameNode) {
1951+
checkClassVariableNameNode.execute(module, name);
19481952
return ModuleOperations.removeClassVariable(module.fields, getContext(), this, name);
19491953
}
19501954

‎src/main/java/org/truffleruby/core/module/ModuleOperations.java

Lines changed: 17 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import java.util.Map.Entry;
1717
import java.util.function.Function;
1818

19+
import com.oracle.truffle.api.object.DynamicObjectLibrary;
1920
import org.truffleruby.RubyContext;
2021
import org.truffleruby.RubyLanguage;
2122
import org.truffleruby.collections.Memo;
@@ -27,6 +28,7 @@
2728
import org.truffleruby.language.control.RaiseException;
2829
import org.truffleruby.language.methods.DeclarationContext;
2930
import org.truffleruby.language.methods.InternalMethod;
31+
import org.truffleruby.language.objects.classvariables.ClassVariableStorage;
3032
import org.truffleruby.language.objects.shared.SharedObjects;
3133
import org.truffleruby.parser.Identifiers;
3234

@@ -614,23 +616,6 @@ private static Assumption[] toArray(ArrayList<Assumption> assumptions) {
614616
return assumptions.toArray(EMPTY_ASSUMPTION_ARRAY);
615617
}
616618

617-
@TruffleBoundary
618-
public static Map<String, Object> getAllClassVariables(RubyModule module) {
619-
final Map<String, Object> classVariables = new HashMap<>();
620-
621-
classVariableLookup(module, module1 -> {
622-
classVariables.putAll(module1.fields.getClassVariables());
623-
return null;
624-
});
625-
626-
return classVariables;
627-
}
628-
629-
@TruffleBoundary
630-
public static Object lookupClassVariable(RubyModule module, final String name) {
631-
return classVariableLookup(module, module1 -> module1.fields.getClassVariables().get(name));
632-
}
633-
634619
@TruffleBoundary
635620
public static void setClassVariable(RubyLanguage language, RubyContext context, RubyModule module, String name,
636621
Object value, Node currentNode) {
@@ -643,48 +628,41 @@ public static void setClassVariable(RubyLanguage language, RubyContext context,
643628
if (!trySetClassVariable(module, name, value)) {
644629
synchronized (context.getClassVariableDefinitionLock()) {
645630
if (!trySetClassVariable(module, name, value)) {
646-
/* This is double-checked locking, but it is safe because when writing to a ConcurrentHashMap "an
647-
* update operation for a given key bears a happens-before relation with any (non-null) retrieval
648-
* for that key reporting the updated value" (JavaDoc) so the value is guaranteed to be fully
649-
* published before it can be found in the map. */
650-
651-
moduleFields.getClassVariables().put(name, value);
631+
moduleFields.getClassVariables().put(name, value, DynamicObjectLibrary.getUncached());
652632
}
653633
}
654634
}
655635
}
656636

657637
private static boolean trySetClassVariable(RubyModule topModule, String name, Object value) {
658-
final RubyModule found = classVariableLookup(topModule, module -> {
659-
final ModuleFields moduleFields = module.fields;
660-
if (moduleFields.getClassVariables().replace(name, value) != null) {
661-
return module;
662-
} else {
663-
return null;
664-
}
665-
});
666-
return found != null;
638+
return classVariableLookup(
639+
topModule,
640+
module -> module.fields.getClassVariables().putIfPresent(
641+
name,
642+
value,
643+
DynamicObjectLibrary.getUncached()) ? module : null) != null;
667644
}
668645

669646
@TruffleBoundary
670-
public static Object removeClassVariable(ModuleFields moduleFields, RubyContext context, Node currentNode,
671-
String name) {
672-
moduleFields.checkFrozen(context, currentNode);
647+
public static Object removeClassVariable(ModuleFields fields, RubyContext context, Node currentNode, String name) {
648+
fields.checkFrozen(context, currentNode);
673649

674-
final Object found = moduleFields.getClassVariables().remove(name);
650+
final ClassVariableStorage classVariables = fields.getClassVariables();
651+
final Object found = classVariables.remove(name, DynamicObjectLibrary.getUncached());
675652
if (found == null) {
676653
throw new RaiseException(
677654
context,
678655
context.getCoreExceptions().nameErrorClassVariableNotDefined(
679656
name,
680-
moduleFields.rubyModule,
657+
fields.rubyModule,
681658
currentNode));
659+
} else {
660+
return found;
682661
}
683-
return found;
684662
}
685663

686664
@TruffleBoundary
687-
private static <R> R classVariableLookup(RubyModule module, Function<RubyModule, R> action) {
665+
public static <R> R classVariableLookup(RubyModule module, Function<RubyModule, R> action) {
688666
// Look in the current module
689667
R result = action.apply(module);
690668
if (result != null) {

‎src/main/java/org/truffleruby/language/LexicalScope.java

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import org.truffleruby.core.module.RubyModule;
1313

1414
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
15-
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
1615

1716
/** Instances of this class represent the Ruby lexical scope for constants, which is only changed by `class Name`,
1817
* `module Name` and `class << expr`. Other lexical scope features such as refinement and the default definee are
@@ -48,18 +47,6 @@ public void unsafeSetLiveModule(RubyModule liveModule) {
4847
this.liveModule = liveModule;
4948
}
5049

51-
@TruffleBoundary
52-
public static RubyModule resolveTargetModuleForClassVariables(LexicalScope lexicalScope) {
53-
LexicalScope scope = lexicalScope;
54-
55-
// MRI logic: ignore lexical scopes (cref) referring to singleton classes
56-
while (RubyGuards.isSingletonClass(scope.liveModule)) {
57-
scope = scope.parent;
58-
}
59-
60-
return scope.liveModule;
61-
}
62-
6350
@Override
6451
public String toString() {
6552
return " :: " + liveModule + (parent == null ? "" : parent.toString());

0 commit comments

Comments
 (0)