Skip to content

Commit 1084d56

Browse files
msimacekmsimacek
committed
Address review feedback
1 parent 6fd5ee4 commit 1084d56

File tree

6 files changed

+66
-152
lines changed

6 files changed

+66
-152
lines changed

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/function/PArguments.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747
* The layout of an argument array for a Python frame.
4848
*
4949
* +-------------------+
50-
* INDEX_VARIABLE_ARGUMENTS -> | Object[] |
50+
* INDEX_VARIABLE_ARGUMENTS -> | Object[] | This slot is also used to pass parent frame reference in bytecode OSR compilation.
5151
* +-------------------+
5252
* INDEX_KEYWORD_ARGUMENTS -> | PKeyword[] |
5353
* +-------------------+

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode/FrameInfo.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,13 @@
4444
import com.oracle.graal.python.compiler.OpCodesConstants;
4545
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
4646
import com.oracle.truffle.api.frame.Frame;
47+
import com.oracle.truffle.api.frame.FrameDescriptor;
4748

49+
/**
50+
* Carries additional metadata for introspection of frames used by the bytecode interpreter. It's
51+
* returned by {@link FrameDescriptor#getInfo()} if and only if the frame is coming from the
52+
* bytecode interpreter.
53+
*/
4854
public final class FrameInfo {
4955
@CompilationFinal PBytecodeRootNode rootNode;
5056

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode/GeneratorResult.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@
4343
import com.oracle.truffle.api.CompilerDirectives.ValueType;
4444

4545
@ValueType
46-
public class GeneratorResult {
46+
public final class GeneratorResult {
4747
public final int resumeBci;
4848
public final int resumeStackTop;
4949
public final boolean isReturn;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode/PrintExprNode.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ public abstract class PrintExprNode extends PNodeWithContext {
6565
void print(VirtualFrame frame, Object object,
6666
@Cached PyObjectLookupAttr lookupAttr,
6767
@Cached CallNode callNode) {
68-
PythonModule sysModule = getContext().lookupBuiltinModule("sys");
68+
PythonModule sysModule = getContext().getSysModule();
6969
Object displayhook = lookupAttr.execute(frame, sysModule, BuiltinNames.DISPLAYHOOK);
7070
if (displayhook == PNone.NO_VALUE) {
7171
CompilerDirectives.transferToInterpreterAndInvalidate();

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/frame/MaterializeFrameNode.java‎

Lines changed: 37 additions & 149 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@
5757
import com.oracle.graal.python.nodes.function.ClassBodyRootNode;
5858
import com.oracle.graal.python.nodes.object.GetClassNode;
5959
import com.oracle.graal.python.runtime.object.PythonObjectFactory;
60+
import com.oracle.graal.python.util.PythonUtils;
6061
import com.oracle.truffle.api.CompilerDirectives;
6162
import com.oracle.truffle.api.dsl.Cached;
6263
import com.oracle.truffle.api.dsl.Cached.Shared;
@@ -295,138 +296,30 @@ public abstract static class SyncFrameValuesNode extends Node {
295296
static void doLocalsStorageCachedAST(PFrame pyFrame, Frame frameToSync, @SuppressWarnings("unused") Node location,
296297
@Cached("createClassProfile()") ValueProfile frameProfile,
297298
@Cached("frameToSync.getFrameDescriptor()") FrameDescriptor cachedFd) {
298-
boolean invalidState = false;
299299
LocalsStorage localsStorage = getLocalsStorage(pyFrame);
300300
MaterializedFrame target = frameProfile.profile(localsStorage.getFrame());
301301
assert cachedFd == target.getFrameDescriptor();
302302

303303
for (int slot = 0; slot < cachedFd.getNumberOfSlots(); slot++) {
304304
if (FrameSlotIDs.isUserFrameSlot(cachedFd.getSlotName(slot))) {
305-
if (frameToSync.isBoolean(slot)) {
306-
try {
307-
target.setBoolean(slot, frameToSync.getBoolean(slot));
308-
} catch (FrameSlotTypeException e) {
309-
CompilerDirectives.transferToInterpreter();
310-
invalidState = true;
311-
}
312-
} else if (frameToSync.isByte(slot)) {
313-
try {
314-
target.setByte(slot, frameToSync.getByte(slot));
315-
} catch (FrameSlotTypeException e) {
316-
CompilerDirectives.transferToInterpreter();
317-
invalidState = true;
318-
}
319-
} else if (frameToSync.isDouble(slot)) {
320-
try {
321-
target.setDouble(slot, frameToSync.getDouble(slot));
322-
} catch (FrameSlotTypeException e) {
323-
CompilerDirectives.transferToInterpreter();
324-
invalidState = true;
325-
}
326-
} else if (frameToSync.isFloat(slot)) {
327-
try {
328-
target.setFloat(slot, frameToSync.getFloat(slot));
329-
} catch (FrameSlotTypeException e) {
330-
CompilerDirectives.transferToInterpreter();
331-
invalidState = true;
332-
}
333-
} else if (frameToSync.isInt(slot)) {
334-
try {
335-
target.setInt(slot, frameToSync.getInt(slot));
336-
} catch (FrameSlotTypeException e) {
337-
CompilerDirectives.transferToInterpreter();
338-
invalidState = true;
339-
}
340-
} else if (frameToSync.isLong(slot)) {
341-
try {
342-
target.setLong(slot, frameToSync.getLong(slot));
343-
} catch (FrameSlotTypeException e) {
344-
CompilerDirectives.transferToInterpreter();
345-
invalidState = true;
346-
}
347-
} else if (frameToSync.isObject(slot)) {
348-
try {
349-
target.setObject(slot, frameToSync.getObject(slot));
350-
} catch (FrameSlotTypeException e) {
351-
CompilerDirectives.transferToInterpreter();
352-
invalidState = true;
353-
}
354-
}
305+
PythonUtils.copyFrameSlot(frameToSync, target, slot);
355306
}
356307
}
357-
if (CompilerDirectives.inInterpreter() && invalidState) {
358-
// we're always in the interpreter if invalidState was set
359-
throw new IllegalStateException("the frame lied about the frame slot type");
360-
}
361308
}
362309

363310
@Specialization(guards = {"!isBytecodeFrame(frameToSync)", "hasLocalsStorage(pyFrame, frameToSync, frameProfile)", "frameToSync.getFrameDescriptor() == cachedFd"}, limit = "1")
364311
static void doLocalsStorageLoopAST(PFrame pyFrame, Frame frameToSync, @SuppressWarnings("unused") Node location,
365312
@Cached("createClassProfile()") ValueProfile frameProfile,
366313
@Cached("frameToSync.getFrameDescriptor()") FrameDescriptor cachedFd) {
367-
boolean invalidState = false;
368314
LocalsStorage localsStorage = getLocalsStorage(pyFrame);
369315
MaterializedFrame target = frameProfile.profile(localsStorage.getFrame());
370316
assert cachedFd == target.getFrameDescriptor();
371317

372318
for (int slot = 0; slot < cachedFd.getNumberOfSlots(); slot++) {
373319
if (FrameSlotIDs.isUserFrameSlot(cachedFd.getSlotName(slot))) {
374-
if (frameToSync.isBoolean(slot)) {
375-
try {
376-
target.setBoolean(slot, frameToSync.getBoolean(slot));
377-
} catch (FrameSlotTypeException e) {
378-
CompilerDirectives.transferToInterpreter();
379-
invalidState = true;
380-
}
381-
} else if (frameToSync.isByte(slot)) {
382-
try {
383-
target.setByte(slot, frameToSync.getByte(slot));
384-
} catch (FrameSlotTypeException e) {
385-
CompilerDirectives.transferToInterpreter();
386-
invalidState = true;
387-
}
388-
} else if (frameToSync.isDouble(slot)) {
389-
try {
390-
target.setDouble(slot, frameToSync.getDouble(slot));
391-
} catch (FrameSlotTypeException e) {
392-
CompilerDirectives.transferToInterpreter();
393-
invalidState = true;
394-
}
395-
} else if (frameToSync.isFloat(slot)) {
396-
try {
397-
target.setFloat(slot, frameToSync.getFloat(slot));
398-
} catch (FrameSlotTypeException e) {
399-
CompilerDirectives.transferToInterpreter();
400-
invalidState = true;
401-
}
402-
} else if (frameToSync.isInt(slot)) {
403-
try {
404-
target.setInt(slot, frameToSync.getInt(slot));
405-
} catch (FrameSlotTypeException e) {
406-
CompilerDirectives.transferToInterpreter();
407-
invalidState = true;
408-
}
409-
} else if (frameToSync.isLong(slot)) {
410-
try {
411-
target.setLong(slot, frameToSync.getLong(slot));
412-
} catch (FrameSlotTypeException e) {
413-
CompilerDirectives.transferToInterpreter();
414-
invalidState = true;
415-
}
416-
} else if (frameToSync.isObject(slot)) {
417-
try {
418-
target.setObject(slot, frameToSync.getObject(slot));
419-
} catch (FrameSlotTypeException e) {
420-
CompilerDirectives.transferToInterpreter();
421-
invalidState = true;
422-
}
423-
}
320+
PythonUtils.copyFrameSlot(frameToSync, target, slot);
424321
}
425322
}
426-
if (CompilerDirectives.inInterpreter() && invalidState) {
427-
// we're always in the interpreter if invalidState was set
428-
throw new IllegalStateException("the frame lied about the frame slot type");
429-
}
430323
}
431324

432325
@Specialization(guards = {"!isBytecodeFrame(frameToSync)", "hasLocalsStorage(pyFrame, frameToSync, frameProfile)"}, replaces = {"doLocalsStorageCachedAST", "doLocalsStorageLoopAST"})
@@ -440,21 +333,7 @@ static void doLocalsStorageUncachedAST(PFrame pyFrame, Frame frameToSync, @Suppr
440333

441334
for (int slot = 0; slot < fd.getNumberOfSlots(); slot++) {
442335
if (FrameSlotIDs.isUserFrameSlot(fd.getSlotName(slot))) {
443-
if (frameToSync.isBoolean(slot)) {
444-
target.setBoolean(slot, frameToSync.getBoolean(slot));
445-
} else if (frameToSync.isByte(slot)) {
446-
target.setByte(slot, frameToSync.getByte(slot));
447-
} else if (frameToSync.isDouble(slot)) {
448-
target.setDouble(slot, frameToSync.getDouble(slot));
449-
} else if (frameToSync.isFloat(slot)) {
450-
target.setFloat(slot, frameToSync.getFloat(slot));
451-
} else if (frameToSync.isInt(slot)) {
452-
target.setInt(slot, frameToSync.getInt(slot));
453-
} else if (frameToSync.isLong(slot)) {
454-
target.setLong(slot, frameToSync.getLong(slot));
455-
} else if (frameToSync.isObject(slot)) {
456-
target.setObject(slot, frameToSync.getObject(slot));
457-
}
336+
PythonUtils.copyFrameSlot(frameToSync, target, slot);
458337
}
459338
}
460339
} catch (FrameSlotTypeException e) {
@@ -560,7 +439,8 @@ static void doCustomLocalsObject(PFrame pyFrame, Frame frameToSync, @SuppressWar
560439
// nothing to do; we already worked on the custom object
561440
}
562441

563-
@Specialization(guards = {"isBytecodeFrame(frameToSync)", "hasLocalsStorage(pyFrame, frameToSync, frameProfile)", "frameToSync.getFrameDescriptor() == cachedFd"}, limit = "1")
442+
@Specialization(guards = {"isBytecodeFrame(frameToSync)", "hasLocalsStorage(pyFrame, frameToSync, frameProfile)", "frameToSync.getFrameDescriptor() == cachedFd",
443+
"variableSlotCount(cachedFd) < 32"}, limit = "1")
564444
@ExplodeLoop
565445
static void doLocalsStorageCachedExploded(PFrame pyFrame, Frame frameToSync, @SuppressWarnings("unused") Node location,
566446
@Cached("createClassProfile()") ValueProfile frameProfile,
@@ -570,7 +450,7 @@ static void doLocalsStorageCachedExploded(PFrame pyFrame, Frame frameToSync, @Su
570450
assert cachedFd == target.getFrameDescriptor();
571451
int slotCount = variableSlotCount(cachedFd);
572452
for (int slot = 0; slot < slotCount; slot++) {
573-
copySlot(frameToSync, target, slot);
453+
PythonUtils.copyFrameSlot(frameToSync, target, slot);
574454
}
575455
}
576456

@@ -584,7 +464,7 @@ static void doLocalsStorageCachedLoop(PFrame pyFrame, Frame frameToSync, @Suppre
584464
assert cachedFd == target.getFrameDescriptor();
585465
int slotCount = variableSlotCount(cachedFd);
586466
for (int slot = 0; slot < slotCount; slot++) {
587-
copySlot(frameToSync, target, slot);
467+
PythonUtils.copyFrameSlot(frameToSync, target, slot);
588468
}
589469
}
590470

@@ -595,17 +475,18 @@ static void doLocalsStorageUncached(PFrame pyFrame, Frame frameToSync, @Suppress
595475
MaterializedFrame target = frameProfile.profile(localsStorage.getFrame());
596476
int slotCount = variableSlotCount(frameToSync.getFrameDescriptor());
597477
for (int slot = 0; slot < slotCount; slot++) {
598-
copySlot(frameToSync, target, slot);
478+
PythonUtils.copyFrameSlot(frameToSync, target, slot);
599479
}
600480
}
601481

602482
@Specialization(guards = { //
603483
"isBytecodeFrame(frameToSync)",
604484
"isDictWithCustomStorage(pyFrame)",
605485
"frameToSync.getFrameDescriptor() == cachedFd",
486+
"variableSlotCount(cachedFd) < 32"
606487
}, limit = "1")
607488
@ExplodeLoop
608-
static void doGenericDictCached(VirtualFrame frame, PFrame pyFrame, Frame frameToSync, @SuppressWarnings("unused") Node location,
489+
static void doGenericDictCachedExploded(VirtualFrame frame, PFrame pyFrame, Frame frameToSync, @SuppressWarnings("unused") Node location,
609490
@Cached("frameToSync.getFrameDescriptor()") @SuppressWarnings("unused") FrameDescriptor cachedFd,
610491
@Cached(value = "getProfiles(variableSlotCount(cachedFd))", dimensions = 1) ConditionProfile[] profiles,
611492
@Cached BranchProfile updatedStorage,
@@ -625,7 +506,32 @@ static void doGenericDictCached(VirtualFrame frame, PFrame pyFrame, Frame frameT
625506
}
626507
}
627508

628-
@Specialization(guards = {"isBytecodeFrame(frameToSync)", "isDictWithCustomStorage(pyFrame)"}, replaces = "doGenericDictCached")
509+
@Specialization(guards = { //
510+
"isBytecodeFrame(frameToSync)",
511+
"isDictWithCustomStorage(pyFrame)",
512+
"frameToSync.getFrameDescriptor() == cachedFd",
513+
}, limit = "1", replaces = "doGenericDictCachedExploded")
514+
static void doGenericDictCachedLoop(VirtualFrame frame, PFrame pyFrame, Frame frameToSync, @SuppressWarnings("unused") Node location,
515+
@Cached("frameToSync.getFrameDescriptor()") @SuppressWarnings("unused") FrameDescriptor cachedFd,
516+
@Cached(value = "getProfiles(variableSlotCount(cachedFd))", dimensions = 1) ConditionProfile[] profiles,
517+
@Cached BranchProfile updatedStorage,
518+
@Cached ConditionProfile hasFrame,
519+
@CachedLibrary(limit = "1") HashingStorageLibrary lib) {
520+
// This can happen if someone received the locals dict using 'locals()' or similar and
521+
// then assigned to the dictionary. Assigning will switch the storage. But we still must
522+
// refresh the values.
523+
524+
// The cast is guaranteed by the guard.
525+
PDict localsDict = (PDict) pyFrame.getLocalsDict();
526+
FrameInfo info = (FrameInfo) cachedFd.getInfo();
527+
int slotCount = info.getVariableCount();
528+
for (int slot = 0; slot < slotCount; slot++) {
529+
ConditionProfile profile = profiles[slot];
530+
syncDict(frame, slot, info, frameToSync, localsDict, lib, hasFrame, updatedStorage, profile);
531+
}
532+
}
533+
534+
@Specialization(guards = {"isBytecodeFrame(frameToSync)", "isDictWithCustomStorage(pyFrame)"}, replaces = {"doGenericDictCachedExploded", "doGenericDictCachedLoop"})
629535
static void doGenericDict(VirtualFrame frame, PFrame pyFrame, Frame frameToSync, @SuppressWarnings("unused") Node location,
630536
@Cached BranchProfile updatedStorage,
631537
@Cached ConditionProfile hasFrame,
@@ -728,23 +634,5 @@ private static Object resolveCellValue(ConditionProfile profile, Object value) {
728634
public static SyncFrameValuesNode create() {
729635
return SyncFrameValuesNodeGen.create();
730636
}
731-
732-
public static void copySlot(Frame frameToSync, MaterializedFrame target, int slot) {
733-
if (frameToSync.isBoolean(slot)) {
734-
target.setBoolean(slot, frameToSync.getBoolean(slot));
735-
} else if (frameToSync.isByte(slot)) {
736-
target.setByte(slot, frameToSync.getByte(slot));
737-
} else if (frameToSync.isDouble(slot)) {
738-
target.setDouble(slot, frameToSync.getDouble(slot));
739-
} else if (frameToSync.isFloat(slot)) {
740-
target.setFloat(slot, frameToSync.getFloat(slot));
741-
} else if (frameToSync.isInt(slot)) {
742-
target.setInt(slot, frameToSync.getInt(slot));
743-
} else if (frameToSync.isLong(slot)) {
744-
target.setLong(slot, frameToSync.getLong(slot));
745-
} else if (frameToSync.isObject(slot)) {
746-
target.setObject(slot, frameToSync.getObject(slot));
747-
}
748-
}
749637
}
750638
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/util/PythonUtils.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,8 @@
7979
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
8080
import com.oracle.truffle.api.RootCallTarget;
8181
import com.oracle.truffle.api.dsl.NodeFactory;
82+
import com.oracle.truffle.api.frame.Frame;
83+
import com.oracle.truffle.api.frame.MaterializedFrame;
8284
import com.oracle.truffle.api.memory.ByteArraySupport;
8385
import com.oracle.truffle.api.nodes.RootNode;
8486
import com.oracle.truffle.api.profiles.ConditionProfile;
@@ -735,4 +737,22 @@ public static Unsafe initUnsafe() {
735737
}
736738
}
737739
}
740+
741+
public static void copyFrameSlot(Frame frameToSync, MaterializedFrame target, int slot) {
742+
if (frameToSync.isObject(slot)) {
743+
target.setObject(slot, frameToSync.getObject(slot));
744+
} else if (frameToSync.isInt(slot)) {
745+
target.setInt(slot, frameToSync.getInt(slot));
746+
} else if (frameToSync.isLong(slot)) {
747+
target.setLong(slot, frameToSync.getLong(slot));
748+
} else if (frameToSync.isBoolean(slot)) {
749+
target.setBoolean(slot, frameToSync.getBoolean(slot));
750+
} else if (frameToSync.isDouble(slot)) {
751+
target.setDouble(slot, frameToSync.getDouble(slot));
752+
} else if (frameToSync.isFloat(slot)) {
753+
target.setFloat(slot, frameToSync.getFloat(slot));
754+
} else if (frameToSync.isByte(slot)) {
755+
target.setByte(slot, frameToSync.getByte(slot));
756+
}
757+
}
738758
}

0 commit comments

Comments
 (0)