Skip to content

Commit 824a359

Browse files
headiusheadius
committed
Copy variable table manager from non-Object parents
There's a few changes here to support Data types and their subclasses: * Always copy the variable table manager from parent to child, unless the parent is Object. This ensures that Data type subclasses still see the same accessors and likely makes sense for most other classes that inherit instance variables. * Fix the definition sequence for Data fields to specialize and properly use the field-based accessors rather than the growable varTable accessors. This maintains a compact store for Data fields where previously the object was specialized but variables still were stored in varTable. This relates to and partially fixes some root causes of the Data type subclass issues reported in #9241. There are other concerns with this structure of Data, however, since it may use accessors from the parent in the child (rather than the cloned versions for the child). We need a larger overhaul of both Data and how the variable table manager is propagated from parents to children (previously no propagation happened and all child classes re-discovered their own instance variables).
1 parent 84538f1 commit 824a359

3 files changed

Lines changed: 21 additions & 20 deletions

File tree

‎core/src/main/java/org/jruby/RubyClass.java‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -550,7 +550,6 @@ public static RubyClass newClass(ThreadContext context, RubyClass superClass, St
550550
baseName(name);
551551
clazz.makeMetaClass(context, superClass.getMetaClass());
552552
if (setParent) clazz.setParent(parent);
553-
clazz.copyVariableTableManagerForData(context, superClass);
554553
parent.setConstant(context, name, clazz, file, line);
555554
superClass.invokeInherited(context, superClass, clazz);
556555
return clazz;
@@ -1073,7 +1072,7 @@ private RubyClass initializeCommon(ThreadContext context, RubyClass superClazz,
10731072
allocator = superClazz.allocator;
10741073
makeMetaClass(context, superClazz.getMetaClass());
10751074
superClazz.addSubclass(this);
1076-
copyVariableTableManagerForData(context, superClazz);
1075+
copyVariableTableManager(superClazz);
10771076

10781077
marshal = superClazz.marshal;
10791078

@@ -1083,11 +1082,14 @@ private RubyClass initializeCommon(ThreadContext context, RubyClass superClazz,
10831082
return this;
10841083
}
10851084

1086-
private void copyVariableTableManagerForData(ThreadContext context, RubyClass superClazz) {
1085+
private void copyVariableTableManager(RubyClass superClazz) {
1086+
if (superClazz == runtime.getObject()) {
1087+
// don't copy variables from Object since it will accumulate random variables
1088+
return;
1089+
}
10871090
VariableTableManager variableTableManager = superClazz.getVariableTableManager();
1088-
if (variableTableManager.getRealClass().superClass() == context.runtime.getData()) {
1089-
// duplicate data's variable table in subclasses
1090-
this.variableTableManager = variableTableManager.duplicateForData(this);
1091+
if (!variableTableManager.getVariableAccessorsForRead().isEmpty()) {
1092+
this.variableTableManager = variableTableManager.duplicateFor(this);
10911093
}
10921094
}
10931095

‎core/src/main/java/org/jruby/RubyData.java‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -402,13 +402,6 @@ private static RubyClass newDataStruct(ThreadContext context, RubyClass superCla
402402

403403
RubyClass subclass = RubyClass.newClass(runtime, superClass);
404404

405-
VariableTableManager vtm = subclass.getVariableTableManager();
406-
VariableAccessor[] accessors = new VariableAccessor[keySet.size()];
407-
int i = 0;
408-
for (RubySymbol sym : keySet) {
409-
accessors[i++] = vtm.getVariableAccessorForWrite(sym.idString());
410-
}
411-
412405
ObjectAllocator allocator =
413406
runtime.getObjectSpecializer()
414407
.specializeForVariables(
@@ -417,6 +410,18 @@ private static RubyClass newDataStruct(ThreadContext context, RubyClass superCla
417410

418411
subclass.allocator(allocator);
419412

413+
VariableTableManager vtm = subclass.getVariableTableManager();
414+
VariableAccessor[] accessors = new VariableAccessor[keySet.size()];
415+
int i = 0;
416+
for (RubySymbol sym : keySet) {
417+
VariableAccessor accessor = vtm.getVariableAccessorForWrite(sym.idString());
418+
419+
accessors[i++] = accessor;
420+
421+
// TODO: AttrReader expects to potentially see many variable tables; this could be simplified
422+
subclass.addMethod(context, sym.idString(), new AttrReaderMethod(subclass, Visibility.PUBLIC, accessor));
423+
}
424+
420425
RubyArray members = newArray(context, keySet);
421426
members.freeze(context);
422427
subclass.setInternalVariable(MEMBERS_KEY, members);
@@ -427,12 +432,6 @@ private static RubyClass newDataStruct(ThreadContext context, RubyClass superCla
427432
dataSClass.undefMethods(context, "define")
428433
.defineMethods(context, DataMethods.class);
429434

430-
for (RubySymbol sym : keySet) {
431-
VariableAccessor accessor = vtm.getVariableAccessorForWrite(sym.idString());
432-
// TODO: AttrReader expects to potentially see many variable tables; this could be simplified
433-
subclass.addMethod(context, sym.idString(), new AttrReaderMethod(subclass, Visibility.PUBLIC, accessor));
434-
}
435-
436435
return subclass;
437436
}
438437

‎core/src/main/java/org/jruby/runtime/ivars/VariableTableManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,7 @@ public Object clearVariable(RubyBasicObject object, String name) {
545545
}
546546
}
547547

548-
public VariableTableManager duplicateForData(RubyClass newRealClass) {
548+
public VariableTableManager duplicateFor(RubyClass newRealClass) {
549549
return new VariableTableManager(this, newRealClass);
550550
}
551551

0 commit comments

Comments
 (0)