From 95588822c77226fb592aae9f83a12ce8fbcf6011 Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Tue, 2 May 2023 01:48:24 -0400 Subject: [PATCH] Add LOAD_METHOD_MODULE This speeds up things like `collections.deque()` where `collections` is a module. Add tests. --- runtime/bytecode.h | 3 +- runtime/ic.cpp | 28 +++++ runtime/ic.h | 10 +- runtime/interpreter-test.cpp | 232 +++++++++++++++++++++++++++++++++++ runtime/interpreter.cpp | 101 ++++++++++++--- runtime/interpreter.h | 2 + 6 files changed, 360 insertions(+), 16 deletions(-) diff --git a/runtime/bytecode.h b/runtime/bytecode.h index 9b0fd18cc..0c9181873 100644 --- a/runtime/bytecode.h +++ b/runtime/bytecode.h @@ -191,7 +191,7 @@ namespace py { V(UNUSED_BYTECODE_171, 171, doInvalidBytecode) \ V(UNUSED_BYTECODE_172, 172, doInvalidBytecode) \ V(UNUSED_BYTECODE_173, 173, doInvalidBytecode) \ - V(UNUSED_BYTECODE_174, 174, doInvalidBytecode) \ + V(LOAD_METHOD_MODULE, 174, doLoadMethodModule) \ V(CALL_FUNCTION_TYPE_INIT, 175, doCallFunctionTypeInit) \ V(CALL_FUNCTION_TYPE_NEW, 176, doCallFunctionTypeNew) \ V(CALL_FUNCTION_ANAMORPHIC, 177, doCallFunctionAnamorphic) \ @@ -393,6 +393,7 @@ inline bool isByteCodeWithCache(const Bytecode bc) { case LOAD_ATTR_INSTANCE_TYPE_BOUND_METHOD: case LOAD_ATTR_INSTANCE_TYPE_DESCR: case LOAD_ATTR_MODULE: + case LOAD_METHOD_MODULE: case LOAD_ATTR_TYPE: case LOAD_ATTR_ANAMORPHIC: case LOAD_METHOD_ANAMORPHIC: diff --git a/runtime/ic.cpp b/runtime/ic.cpp index 5d5998b2d..7e5ebaeff 100644 --- a/runtime/ic.cpp +++ b/runtime/ic.cpp @@ -120,6 +120,25 @@ void icUpdateAttrModule(Thread* thread, const MutableTuple& caches, word cache, icInsertDependentToValueCellDependencyLink(thread, dependent, value_cell); } +void icUpdateMethodModule(Thread* thread, const MutableTuple& caches, + word cache, const Object& receiver, + const ValueCell& value_cell, + const Function& dependent) { + DCHECK(icIsCacheEmpty(caches, cache), "cache must be empty\n"); + HandleScope scope(thread); + word index = cache * kIcPointersPerEntry; + Module module(&scope, *receiver); + caches.atPut(index + kIcEntryKeyOffset, SmallInt::fromWord(module.id())); + caches.atPut(index + kIcEntryValueOffset, *value_cell); + RawMutableBytes bytecode = + RawMutableBytes::cast(dependent.rewrittenBytecode()); + word pc = thread->currentFrame()->virtualPC() - kCodeUnitSize; + DCHECK(bytecode.byteAt(pc) == LOAD_METHOD_ANAMORPHIC, + "current opcode must be LOAD_METHOD_ANAMORPHIC"); + bytecode.byteAtPut(pc, LOAD_METHOD_MODULE); + icInsertDependentToValueCellDependencyLink(thread, dependent, value_cell); +} + void icUpdateAttrType(Thread* thread, const MutableTuple& caches, word cache, const Object& receiver, const Object& selector, const Object& value, const Function& dependent) { @@ -822,6 +841,15 @@ void icInvalidateGlobalVar(Thread* thread, const ValueCell& value_cell) { } break; } + case LOAD_METHOD_MODULE: { + original_bc = LOAD_METHOD_ANAMORPHIC; + word index = op.cache * kIcPointersPerEntry; + if (caches.at(index + kIcEntryValueOffset) == *value_cell) { + caches.atPut(index + kIcEntryKeyOffset, NoneType::object()); + caches.atPut(index + kIcEntryValueOffset, NoneType::object()); + } + break; + } case LOAD_GLOBAL_CACHED: original_bc = LOAD_GLOBAL; if (op.bc != original_bc && op.arg == name_index_found) { diff --git a/runtime/ic.h b/runtime/ic.h index 129c5aaf9..5d42ac99c 100644 --- a/runtime/ic.h +++ b/runtime/ic.h @@ -63,6 +63,11 @@ void icUpdateAttrModule(Thread* thread, const MutableTuple& caches, word cache, const Object& receiver, const ValueCell& value_cell, const Function& dependent); +void icUpdateMethodModule(Thread* thread, const MutableTuple& caches, + word cache, const Object& receiver, + const ValueCell& value_cell, + const Function& dependent); + void icUpdateAttrType(Thread* thread, const MutableTuple& caches, word cache, const Object& receiver, const Object& selector, const Object& value, const Function& dependent); @@ -361,7 +366,10 @@ class IcIterator { } } - bool isModuleAttrCache() const { return bytecode_op_.bc == LOAD_ATTR_MODULE; } + bool isModuleAttrCache() const { + return bytecode_op_.bc == LOAD_ATTR_MODULE || + bytecode_op_.bc == LOAD_METHOD_MODULE; + } bool isBinaryOpCache() const { switch (bytecode_op_.bc) { diff --git a/runtime/interpreter-test.cpp b/runtime/interpreter-test.cpp index 5c90f3fc2..6dd186968 100644 --- a/runtime/interpreter-test.cpp +++ b/runtime/interpreter-test.cpp @@ -6737,6 +6737,238 @@ c = C() EXPECT_TRUE(isIntEqualsWord(Interpreter::call0(thread_, test_function), 70)); } +TEST_F(InterpreterTest, LoadMethodCachedModuleFunction) { + EXPECT_FALSE(runFromCStr(runtime_, R"( +import sys + +class C: + def getdefaultencoding(self): + return "no-utf8" + +def test(obj): + return obj.getdefaultencoding() + +cached = sys.getdefaultencoding +obj = C() +)") + .isError()); + HandleScope scope(thread_); + Function test_function(&scope, mainModuleAt(runtime_, "test")); + Function expected_value(&scope, mainModuleAt(runtime_, "cached")); + MutableBytes bytecode(&scope, test_function.rewrittenBytecode()); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_ANAMORPHIC); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 2), CALL_METHOD); + + // Cache miss. + Module sys_module(&scope, runtime_->findModuleById(ID(sys))); + MutableTuple caches(&scope, test_function.caches()); + word cache_index = + rewrittenBytecodeCacheAt(bytecode, 1) * kIcPointersPerEntry; + Object key(&scope, caches.at(cache_index + kIcEntryKeyOffset)); + EXPECT_EQ(*key, NoneType::object()); + + // Call. + EXPECT_TRUE(isStrEqualsCStr( + Interpreter::call1(thread_, test_function, sys_module), "utf-8")); + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_MODULE); + + // Cache hit. + key = caches.at(cache_index + kIcEntryKeyOffset); + EXPECT_TRUE(isIntEqualsWord(*key, sys_module.id())); + Object value(&scope, caches.at(cache_index + kIcEntryValueOffset)); + ASSERT_TRUE(value.isValueCell()); + EXPECT_EQ(ValueCell::cast(*value).value(), *expected_value); + + // Call. + EXPECT_TRUE(isStrEqualsCStr( + Interpreter::call1(thread_, test_function, sys_module), "utf-8")); + + // Rewrite. + Object obj(&scope, mainModuleAt(runtime_, "obj")); + EXPECT_TRUE(isStrEqualsCStr(Interpreter::call1(thread_, test_function, obj), + "no-utf8")); + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_INSTANCE_FUNCTION); + key = caches.at(cache_index + kIcEntryKeyOffset); + EXPECT_FALSE(key.isValueCell()); +} + +TEST_F(InterpreterTest, + LoadMethodWithModuleAndNonFunctionRewritesToLoadMethodModule) { + EXPECT_FALSE(runFromCStr(runtime_, R"( +import sys + +class C: + def __call__(self): + return 123 + +mymodule = type(sys)("mymodule") +mymodule.getdefaultencoding = C() + +def test(obj): + return obj.getdefaultencoding() +)") + .isError()); + HandleScope scope(thread_); + Function test_function(&scope, mainModuleAt(runtime_, "test")); + MutableBytes bytecode(&scope, test_function.rewrittenBytecode()); + Module mymodule(&scope, mainModuleAt(runtime_, "mymodule")); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_ANAMORPHIC); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 2), CALL_METHOD); + + // Cache miss. + MutableTuple caches(&scope, test_function.caches()); + word cache_index = + rewrittenBytecodeCacheAt(bytecode, 1) * kIcPointersPerEntry; + Object key(&scope, caches.at(cache_index + kIcEntryKeyOffset)); + EXPECT_EQ(*key, NoneType::object()); + + // Call. + EXPECT_TRUE(isIntEqualsWord( + Interpreter::call1(thread_, test_function, mymodule), 123)); + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_MODULE); +} + +TEST_F(InterpreterTest, LoadMethodModuleGetsEvicted) { + EXPECT_FALSE(runFromCStr(runtime_, R"( +import sys + +def test(obj): + return obj.getdefaultencoding() +)") + .isError()); + HandleScope scope(thread_); + Function test_function(&scope, mainModuleAt(runtime_, "test")); + MutableBytes bytecode(&scope, test_function.rewrittenBytecode()); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_ANAMORPHIC); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 2), CALL_METHOD); + + // Cache miss. + Module sys_module(&scope, runtime_->findModuleById(ID(sys))); + MutableTuple caches(&scope, test_function.caches()); + word cache_index = + rewrittenBytecodeCacheAt(bytecode, 1) * kIcPointersPerEntry; + Object key(&scope, caches.at(cache_index + kIcEntryKeyOffset)); + EXPECT_EQ(*key, NoneType::object()); + + // Call. + EXPECT_TRUE(isStrEqualsCStr( + Interpreter::call1(thread_, test_function, sys_module), "utf-8")); + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_MODULE); + + // Update module. + Str getdefaultencoding( + &scope, runtime_->internStrFromCStr(thread_, "getdefaultencoding")); + Object result(&scope, + moduleDeleteAttribute(thread_, sys_module, getdefaultencoding)); + ASSERT_TRUE(result.isNoneType()); + + // Cache is empty. + key = caches.at(cache_index + kIcEntryKeyOffset); + EXPECT_TRUE(key.isNoneType()); + + // Cache miss. + EXPECT_TRUE( + raisedWithStr(Interpreter::call1(thread_, test_function, sys_module), + LayoutId::kAttributeError, + "module 'sys' has no attribute 'getdefaultencoding'")); + + // Bytecode gets rewritten after next call. + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_ANAMORPHIC); +} + +TEST_F(InterpreterTest, LoadMethodModuleWithModuleMismatchUpdatesCache) { + EXPECT_FALSE(runFromCStr(runtime_, R"( +import sys + +mymodule = type(sys)("mymodule") +mymodule.getdefaultencoding = lambda: "hello" + +def test(obj): + return obj.getdefaultencoding() +)") + .isError()); + HandleScope scope(thread_); + Function test_function(&scope, mainModuleAt(runtime_, "test")); + Module mymodule(&scope, mainModuleAt(runtime_, "mymodule")); + MutableBytes bytecode(&scope, test_function.rewrittenBytecode()); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_ANAMORPHIC); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 2), CALL_METHOD); + + // Cache miss. + Module sys_module(&scope, runtime_->findModuleById(ID(sys))); + MutableTuple caches(&scope, test_function.caches()); + word cache_index = + rewrittenBytecodeCacheAt(bytecode, 1) * kIcPointersPerEntry; + Object key(&scope, caches.at(cache_index + kIcEntryKeyOffset)); + EXPECT_EQ(*key, NoneType::object()); + + // Call. + EXPECT_TRUE(isStrEqualsCStr( + Interpreter::call1(thread_, test_function, sys_module), "utf-8")); + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_MODULE); + + // Cache contains sys. + key = caches.at(cache_index + kIcEntryKeyOffset); + EXPECT_TRUE(isIntEqualsWord(*key, sys_module.id())); + + // Call. + EXPECT_TRUE(isStrEqualsCStr( + Interpreter::call1(thread_, test_function, mymodule), "hello")); + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_METHOD_MODULE); + + // Cache contains mymodule. + key = caches.at(cache_index + kIcEntryKeyOffset); + EXPECT_TRUE(isIntEqualsWord(*key, mymodule.id())); +} + +TEST_F(InterpreterTest, LoadMethodModuleGetsScannedInOtherEviction) { + EXPECT_FALSE(runFromCStr(runtime_, R"( +import sys + +class C: + def __init__(self): + self.foo = 123 + +c = C() + +def test(obj): + c.foo + return obj.getdefaultencoding() + +def invalidate(): + C.foo = property(lambda self: 456) +)") + .isError()); + HandleScope scope(thread_); + Function test_function(&scope, mainModuleAt(runtime_, "test")); + Function invalidate(&scope, mainModuleAt(runtime_, "invalidate")); + MutableBytes bytecode(&scope, test_function.rewrittenBytecode()); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 4), LOAD_METHOD_ANAMORPHIC); + ASSERT_EQ(rewrittenBytecodeOpAt(bytecode, 5), CALL_METHOD); + + // Cache miss. + Module sys_module(&scope, runtime_->findModuleById(ID(sys))); + MutableTuple caches(&scope, test_function.caches()); + word cache_index = + rewrittenBytecodeCacheAt(bytecode, 4) * kIcPointersPerEntry; + Object key(&scope, caches.at(cache_index + kIcEntryKeyOffset)); + EXPECT_EQ(*key, NoneType::object()); + + // Call. + EXPECT_TRUE(isStrEqualsCStr( + Interpreter::call1(thread_, test_function, sys_module), "utf-8")); + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 4), LOAD_METHOD_MODULE); + + // Evict the caches in the `test' function. + ASSERT_TRUE(Interpreter::call0(thread_, invalidate).isNoneType()); + + // The LOAD_METHOD_MODULE is not affected. + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 4), LOAD_METHOD_MODULE); + EXPECT_TRUE(isStrEqualsCStr( + Interpreter::call1(thread_, test_function, sys_module), "utf-8")); + EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 4), LOAD_METHOD_MODULE); +} + TEST_F(InterpreterTest, LoadMethodCachedDoesNotCacheProperty) { HandleScope scope(thread_); EXPECT_FALSE(runFromCStr(runtime_, R"( diff --git a/runtime/interpreter.cpp b/runtime/interpreter.cpp index ac1e19f4c..6ef49b511 100644 --- a/runtime/interpreter.cpp +++ b/runtime/interpreter.cpp @@ -3971,6 +3971,8 @@ HANDLER_INLINE Continue Interpreter::doLoadAttrModule(Thread* thread, SmallInt::fromWord(receiver.rawCast().id()) == cache_key) { RawObject result = caches.at(index + kIcEntryValueOffset); DCHECK(result.isValueCell(), "cached value is not a value cell"); + DCHECK(!ValueCell::cast(result).isPlaceholder(), + "attribute has been deleted"); thread->stackSetTop(ValueCell::cast(result).value()); return Continue::NEXT; } @@ -5273,7 +5275,8 @@ Continue Interpreter::loadMethodUpdateCache(Thread* thread, word arg, Object result(&scope, thread->runtime()->attributeAtSetLocation( thread, receiver, name, &kind, &location)); if (result.isErrorException()) return Continue::UNWIND; - if (kind != LoadAttrKind::kInstanceFunction) { + if (kind != LoadAttrKind::kInstanceFunction && + kind != LoadAttrKind::kModule) { thread->stackPush(*result); thread->stackSetAt(1, Unbound::object()); return Continue::NEXT; @@ -5281,21 +5284,44 @@ Continue Interpreter::loadMethodUpdateCache(Thread* thread, word arg, // Cache the attribute load. MutableTuple caches(&scope, frame->caches()); - ICState next_ic_state = icUpdateAttr( - thread, caches, cache, receiver.layoutId(), location, name, dependent); + ICState ic_state = icCurrentState(*caches, cache); - switch (next_ic_state) { - case ICState::kMonomorphic: - rewriteCurrentBytecode(frame, LOAD_METHOD_INSTANCE_FUNCTION); - break; - case ICState::kPolymorphic: - rewriteCurrentBytecode(frame, LOAD_METHOD_POLYMORPHIC); - break; - case ICState::kAnamorphic: - UNREACHABLE("next_ic_state cannot be anamorphic"); - break; + if (ic_state == ICState::kAnamorphic) { + switch (kind) { + case LoadAttrKind::kInstanceFunction: + rewriteCurrentBytecode(frame, LOAD_METHOD_INSTANCE_FUNCTION); + icUpdateAttr(thread, caches, cache, receiver.layoutId(), location, name, + dependent); + break; + case LoadAttrKind::kModule: { + DCHECK(location.isValueCell(), "location must be ValueCell"); + ValueCell value_cell(&scope, *location); + icUpdateMethodModule(thread, caches, cache, receiver, value_cell, + dependent); + thread->stackPush(*result); + thread->stackSetAt(1, Unbound::object()); + return Continue::NEXT; + } + default: + break; + } + } else { + DCHECK(currentBytecode(thread) == LOAD_METHOD_INSTANCE_FUNCTION || + currentBytecode(thread) == LOAD_METHOD_MODULE || + currentBytecode(thread) == LOAD_METHOD_POLYMORPHIC, + "unexpected opcode %s", kBytecodeNames[currentBytecode(thread)]); + switch (kind) { + case LoadAttrKind::kInstanceFunction: + rewriteCurrentBytecode(frame, LOAD_METHOD_POLYMORPHIC); + icUpdateAttr(thread, caches, cache, receiver.layoutId(), location, name, + dependent); + break; + default: + break; + } } - thread->stackInsertAt(1, *location); + thread->stackPush(*result); + thread->stackSetAt(1, Unbound::object()); return Continue::NEXT; } @@ -5305,6 +5331,53 @@ HANDLER_INLINE Continue Interpreter::doLoadMethodAnamorphic(Thread* thread, return loadMethodUpdateCache(thread, arg, cache); } +// This code cleans-up a monomorphic cache and prepares it for its potential +// use as a polymorphic cache. This code should be removed when we change the +// structure of our caches directly accessible from a function to be monomophic +// and to allocate the relatively uncommon polymorphic caches in a separate +// object. +NEVER_INLINE Continue Interpreter::retryLoadMethodCached(Thread* thread, + word arg, word cache) { + // Revert the opcode, clear the cache, and retry the attribute lookup. + Frame* frame = thread->currentFrame(); + if (frame->function().isCompiled()) { + return Continue::DEOPT; + } + rewriteCurrentBytecode(frame, LOAD_METHOD_ANAMORPHIC); + RawMutableTuple caches = MutableTuple::cast(frame->caches()); + word index = cache * kIcPointersPerEntry; + caches.atPut(index + kIcEntryKeyOffset, NoneType::object()); + caches.atPut(index + kIcEntryValueOffset, NoneType::object()); + return Interpreter::loadMethodUpdateCache(thread, arg, cache); +} + +HANDLER_INLINE Continue Interpreter::doLoadMethodModule(Thread* thread, + word arg) { + Frame* frame = thread->currentFrame(); + RawObject receiver = thread->stackTop(); + RawMutableTuple caches = MutableTuple::cast(frame->caches()); + word cache = currentCacheIndex(frame); + word index = cache * kIcPointersPerEntry; + RawObject cache_key = caches.at(index + kIcEntryKeyOffset); + // isInstanceOfModule() should be just as fast as isModule() in the common + // case. If code size or quality is an issue we can adjust this as needed + // based on the types that actually flow through here. + if (thread->runtime()->isInstanceOfModule(receiver) && + // Use rawCast() to support subclasses without the overhead of a + // handle. + SmallInt::fromWord(receiver.rawCast().id()) == cache_key) { + RawObject result = caches.at(index + kIcEntryValueOffset); + DCHECK(result.isValueCell(), "cached value is not a value cell"); + DCHECK(!ValueCell::cast(result).isPlaceholder(), + "attribute has been deleted"); + thread->stackPush(ValueCell::cast(result).value()); + thread->stackSetAt(1, Unbound::object()); + return Continue::NEXT; + } + EVENT_CACHE(LOAD_METHOD_MODULE); + return retryLoadMethodCached(thread, arg, cache); +} + HANDLER_INLINE Continue Interpreter::doLoadMethodInstanceFunction(Thread* thread, word arg) { Frame* frame = thread->currentFrame(); diff --git a/runtime/interpreter.h b/runtime/interpreter.h index 44133931a..91c487d5e 100644 --- a/runtime/interpreter.h +++ b/runtime/interpreter.h @@ -470,6 +470,7 @@ class Interpreter { static Continue doLoadMethod(Thread* thread, word arg); static Continue doLoadMethodAnamorphic(Thread* thread, word arg); static Continue doLoadMethodInstanceFunction(Thread* thread, word arg); + static Continue doLoadMethodModule(Thread* thread, word arg); static Continue doLoadMethodPolymorphic(Thread* thread, word arg); static Continue doLoadName(Thread* thread, word arg); static Continue doLoadType(Thread* thread, word arg); @@ -632,6 +633,7 @@ class Interpreter { word nargs, RawObject* post_call_sp); static Continue retryLoadAttrCached(Thread* thread, word arg, word cache); + static Continue retryLoadMethodCached(Thread* thread, word arg, word cache); static Continue loadAttrUpdateCache(Thread* thread, word arg, word cache); static Continue storeAttrUpdateCache(Thread* thread, word arg, word cache); static Continue storeSubscr(Thread* thread, RawObject set_item_method);