From 9ddf1c8fb7cc6a462331fcefbcb3a2d7e09a1c8c Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Tue, 2 May 2023 13:08:31 -0400 Subject: [PATCH 1/3] Empty overflowing polymorphic caches Right now we have no notion of a megamorphic cache, so when a polymorphic cache is filled, any new receiver LayoutIds fall off the end into the slow full attribute lookup. With this change, polymorphic cache updates will remove the existing cache entries and add the new entry. Add tests. --- runtime/ic.cpp | 9 ++++ runtime/interpreter-test.cpp | 89 ++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+) diff --git a/runtime/ic.cpp b/runtime/ic.cpp index a7277028c..b8863708c 100644 --- a/runtime/ic.cpp +++ b/runtime/ic.cpp @@ -80,15 +80,24 @@ ICState icUpdateAttr(Thread* thread, const MutableTuple& caches, word cache, } RawMutableTuple polymorphic_cache = MutableTuple::cast(caches.at(index + kIcEntryValueOffset)); + bool found = false; for (word j = 0; j < kIcPointersPerPolyCache; j += kIcPointersPerEntry) { entry_key = polymorphic_cache.at(j + kIcEntryKeyOffset); if (entry_key.isNoneType() || entry_key == key) { polymorphic_cache.atPut(j + kIcEntryKeyOffset, key); polymorphic_cache.atPut(j + kIcEntryValueOffset, *value); insertDependencyForTypeLookupInMro(thread, layout_id, name, dependent); + found = true; break; } } + if (!found) { + // Start over. Empty the cache. + polymorphic_cache.fill(NoneType::object()); + polymorphic_cache.atPut(kIcEntryKeyOffset, key); + polymorphic_cache.atPut(kIcEntryValueOffset, *value); + insertDependencyForTypeLookupInMro(thread, layout_id, name, dependent); + } return ICState::kPolymorphic; } diff --git a/runtime/interpreter-test.cpp b/runtime/interpreter-test.cpp index a4c66ab4b..03df16ae8 100644 --- a/runtime/interpreter-test.cpp +++ b/runtime/interpreter-test.cpp @@ -6664,6 +6664,95 @@ c = C() EXPECT_EQ(rewrittenBytecodeOpAt(bytecode, 1), LOAD_ATTR_INSTANCE_PROPERTY); } +TEST_F(InterpreterTest, LoadAttrPolymorphicRewritesEntries) { + EXPECT_FALSE(runFromCStr(runtime_, R"( +class A: + def __init__(self): + self.foo = 400 + +class B(A): + pass + +class C(A): + pass + +class D(A): + pass + +class E(A): + pass + +def cache_attribute(c): + return c.foo + +a = A() +b = B() +c = C() +d = D() +e = E() +)") + .isError()); + HandleScope scope(thread_); + Object a(&scope, mainModuleAt(runtime_, "a")); + Object b(&scope, mainModuleAt(runtime_, "b")); + Object c(&scope, mainModuleAt(runtime_, "c")); + Object d(&scope, mainModuleAt(runtime_, "d")); + Object e(&scope, mainModuleAt(runtime_, "e")); + Function cache_attribute(&scope, mainModuleAt(runtime_, "cache_attribute")); + MutableTuple caches(&scope, cache_attribute.caches()); + ASSERT_EQ(caches.length(), 2 * kIcPointersPerEntry); + + // Load the cache for `a'. + ASSERT_TRUE(icLookupAttr(*caches, 1, a.layoutId()).isErrorNotFound()); + ASSERT_TRUE(icLookupAttr(*caches, 1, b.layoutId()).isErrorNotFound()); + ASSERT_TRUE(icLookupAttr(*caches, 1, c.layoutId()).isErrorNotFound()); + ASSERT_TRUE(icLookupAttr(*caches, 1, d.layoutId()).isErrorNotFound()); + ASSERT_TRUE(icLookupAttr(*caches, 1, e.layoutId()).isErrorNotFound()); + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, a), 400)); + EXPECT_TRUE(icLookupAttr(*caches, 1, a.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, b.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, c.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, d.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, e.layoutId()).isErrorNotFound()); + + // Load the cache for `b'. + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, b), 400)); + EXPECT_TRUE(icLookupAttr(*caches, 1, a.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, b.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, c.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, d.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, e.layoutId()).isErrorNotFound()); + + // Load the cache for `c'. + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, c), 400)); + EXPECT_TRUE(icLookupAttr(*caches, 1, a.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, b.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, c.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, d.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, e.layoutId()).isErrorNotFound()); + + // Load the cache for `d'. + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, d), 400)); + EXPECT_TRUE(icLookupAttr(*caches, 1, a.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, b.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, c.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, d.layoutId()).isSmallInt()); + EXPECT_TRUE(icLookupAttr(*caches, 1, e.layoutId()).isErrorNotFound()); + + // Empty the cache and add `e'. + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, e), 400)); + EXPECT_TRUE(icLookupAttr(*caches, 1, a.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, b.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, c.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, d.layoutId()).isErrorNotFound()); + EXPECT_TRUE(icLookupAttr(*caches, 1, e.layoutId()).isSmallInt()); +} + TEST_F(InterpreterTest, StoreAttrCachedInsertsExecutingFunctionAsDependent) { EXPECT_FALSE(runFromCStr(runtime_, R"( class C: From f075ecdc1fedb2e668db08226cbcb67f5cb396ae Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Wed, 3 May 2023 02:26:02 -0400 Subject: [PATCH 2/3] Also empty binop overflowing polymorphic ICs --- runtime/ic.cpp | 8 +++ runtime/interpreter-test.cpp | 120 +++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) diff --git a/runtime/ic.cpp b/runtime/ic.cpp index b8863708c..d496bd0c4 100644 --- a/runtime/ic.cpp +++ b/runtime/ic.cpp @@ -736,15 +736,23 @@ ICState icUpdateBinOp(Thread* thread, const MutableTuple& caches, word cache, } RawMutableTuple polymorphic_cache = MutableTuple::cast(caches.at(index + kIcEntryValueOffset)); + bool found = false; for (word j = 0; j < kIcPointersPerPolyCache; j += kIcPointersPerEntry) { entry_key = polymorphic_cache.at(j + kIcEntryKeyOffset); if (entry_key.isNoneType() || SmallInt::cast(entry_key).value() >> kBitsPerByte == key_high_bits) { polymorphic_cache.atPut(j + kIcEntryKeyOffset, new_key); polymorphic_cache.atPut(j + kIcEntryValueOffset, *value); + found = true; break; } } + if (!found) { + // Start over. Empty the cache. + polymorphic_cache.fill(NoneType::object()); + polymorphic_cache.atPut(kIcEntryKeyOffset, new_key); + polymorphic_cache.atPut(kIcEntryValueOffset, *value); + } return ICState::kPolymorphic; } diff --git a/runtime/interpreter-test.cpp b/runtime/interpreter-test.cpp index 03df16ae8..168b80b21 100644 --- a/runtime/interpreter-test.cpp +++ b/runtime/interpreter-test.cpp @@ -6753,6 +6753,126 @@ e = E() EXPECT_TRUE(icLookupAttr(*caches, 1, e.layoutId()).isSmallInt()); } +TEST_F(InterpreterTest, BinOpPolymorphicRewritesEntries) { + EXPECT_FALSE(runFromCStr(runtime_, R"( +class A: + def __add__(self, other): + return 123 + +class B(A): + pass + +class C(A): + pass + +class D(A): + pass + +class E(A): + pass + +def cache_attribute(c): + return c + c + +a = A() +b = B() +c = C() +d = D() +e = E() +)") + .isError()); + HandleScope scope(thread_); + Object a(&scope, mainModuleAt(runtime_, "a")); + Object b(&scope, mainModuleAt(runtime_, "b")); + Object c(&scope, mainModuleAt(runtime_, "c")); + Object d(&scope, mainModuleAt(runtime_, "d")); + Object e(&scope, mainModuleAt(runtime_, "e")); + Function cache_attribute(&scope, mainModuleAt(runtime_, "cache_attribute")); + MutableTuple caches(&scope, cache_attribute.caches()); + ASSERT_EQ(caches.length(), kIcPointersPerEntry); + BinaryOpFlags flags; + + // Load the cache for `a'. + ASSERT_TRUE(icLookupBinaryOp(*caches, 0, a.layoutId(), a.layoutId(), &flags) + .isErrorNotFound()); + ASSERT_TRUE(icLookupBinaryOp(*caches, 0, b.layoutId(), b.layoutId(), &flags) + .isErrorNotFound()); + ASSERT_TRUE(icLookupBinaryOp(*caches, 0, c.layoutId(), c.layoutId(), &flags) + .isErrorNotFound()); + ASSERT_TRUE(icLookupBinaryOp(*caches, 0, d.layoutId(), d.layoutId(), &flags) + .isErrorNotFound()); + ASSERT_TRUE(icLookupBinaryOp(*caches, 0, e.layoutId(), e.layoutId(), &flags) + .isErrorNotFound()); + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, a), 123)); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, a.layoutId(), a.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, b.layoutId(), b.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, c.layoutId(), c.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, d.layoutId(), d.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, e.layoutId(), e.layoutId(), &flags) + .isErrorNotFound()); + + // Load the cache for `b'. + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, b), 123)); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, a.layoutId(), a.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, b.layoutId(), b.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, c.layoutId(), c.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, d.layoutId(), d.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, e.layoutId(), e.layoutId(), &flags) + .isErrorNotFound()); + + // Load the cache for `c'. + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, c), 123)); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, a.layoutId(), a.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, b.layoutId(), b.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, c.layoutId(), c.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, d.layoutId(), d.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, e.layoutId(), e.layoutId(), &flags) + .isErrorNotFound()); + + // Load the cache for `d'. + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, d), 123)); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, a.layoutId(), a.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, b.layoutId(), b.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, c.layoutId(), c.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, d.layoutId(), d.layoutId(), &flags) + .isFunction()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, e.layoutId(), e.layoutId(), &flags) + .isErrorNotFound()); + + // Empty the cache and add `e'. + ASSERT_TRUE( + isIntEqualsWord(Interpreter::call1(thread_, cache_attribute, e), 123)); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, a.layoutId(), a.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, b.layoutId(), b.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, c.layoutId(), c.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, d.layoutId(), d.layoutId(), &flags) + .isErrorNotFound()); + EXPECT_TRUE(icLookupBinaryOp(*caches, 0, e.layoutId(), e.layoutId(), &flags) + .isFunction()); +} + TEST_F(InterpreterTest, StoreAttrCachedInsertsExecutingFunctionAsDependent) { EXPECT_FALSE(runFromCStr(runtime_, R"( class C: From 901ccc04e05ad5918ac4ec4ed0525291c96ab3fb Mon Sep 17 00:00:00 2001 From: Max Bernstein Date: Wed, 3 May 2023 02:29:42 -0400 Subject: [PATCH 3/3] Update IC miss probe to track misses by event type --- util/probes/ic-miss.bt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util/probes/ic-miss.bt b/util/probes/ic-miss.bt index 66b879a20..bb7521b47 100644 --- a/util/probes/ic-miss.bt +++ b/util/probes/ic-miss.bt @@ -1,4 +1,4 @@ // Copyright (c) Facebook, Inc. and its affiliates. (http://www.facebook.com) -usdt:./build-debugopt/bin/python:python:InvalidateInlineCache_* { - @ic_miss[ustack()]++; +usdt:./build/bin/python:python:InvalidateInlineCache_* { + @ic_miss[probe]++; }