Skip to content

Commit bcffd43

Browse files
alexmalyshevfacebook-github-bot
authored andcommitted
Have functions own their deopt patchers
Summary: Currently deopt patchers are all owned by the Runtime singleton. They are stored in a single vector structure. They're technically all TypeDeoptPatchers underneath today. The way they get accessed is that when a type is modified, the Runtime singleton checks a Map[Type, Vector[TypeDeoptPatcher*]] structure and then patches the respective patchers depending on the type. This means that all deopt patchers live as long as the Runtime does. There is no deallocation path for individual patchers today. Move the deopt patchers onto CompiledFunction objects, keeping them on the HIR function during compilation. This will allow them to be destroyed when individual functions are destroyed, and it gives us a way to traverse all patchers for an individual function. It also allows us to prune unreachable / unused patchers after codegen is complete. Reviewed By: mpage Differential Revision: D83502081 fbshipit-source-id: 68dece60649241c26f2ea48035d7b3a4f7568c6f
1 parent a8cb0d1 commit bcffd43

8 files changed

Lines changed: 42 additions & 17 deletions

File tree

cinderx/Jit/codegen/gen_asm.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1429,6 +1429,14 @@ void NativeGenerator::linkDeoptPatchers(const asmjit::CodeHolder& code) {
14291429
env_.rt->watchType(typed_patcher->type(), typed_patcher);
14301430
}
14311431
}
1432+
1433+
// Any patchers that aren't linked at this point are pointing to patch points
1434+
// that were optimized out. It's safe to delete them.
1435+
std::erase_if(
1436+
const_cast<hir::Function*>(func_)->deopt_patchers,
1437+
[](std::unique_ptr<DeoptPatcher>& patcher) {
1438+
return !patcher->isLinked();
1439+
});
14321440
}
14331441

14341442
void NativeGenerator::generateResumeEntry(const FrameInfo& frame_info) {

cinderx/Jit/compiled_function.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ void CompiledFunction::setCompileTime(std::chrono::nanoseconds time) {
5858
compile_time_ = time;
5959
}
6060

61+
void CompiledFunction::setDeoptPatchers(
62+
std::vector<std::unique_ptr<DeoptPatcher>>&& deopt_patchers) {
63+
deopt_patchers_ = std::move(deopt_patchers);
64+
}
65+
6166
void CompiledFunction::setHirFunc(std::unique_ptr<hir::Function>&& irfunc) {
6267
irfunc_ = std::move(irfunc);
6368
}

cinderx/Jit/compiled_function.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ bool isJitCompiled(const PyFunctionObject* func);
5555

5656
#include "cinderx/Common/util.h"
5757
#include "cinderx/Jit/code_runtime.h"
58+
#include "cinderx/Jit/deopt_patcher.h"
5859
#include "cinderx/Jit/hir/hir.h"
5960

6061
#include <chrono>
@@ -129,6 +130,9 @@ class CompiledFunction {
129130
std::chrono::nanoseconds compileTime() const;
130131
void setCompileTime(std::chrono::nanoseconds time);
131132

133+
void setDeoptPatchers(
134+
std::vector<std::unique_ptr<DeoptPatcher>>&& deopt_patchers);
135+
132136
void setHirFunc(std::unique_ptr<hir::Function>&& irfunc);
133137

134138
const hir::Function::InlineFunctionStats& inlinedFunctionsStats() const {
@@ -150,6 +154,8 @@ class CompiledFunction {
150154
std::chrono::nanoseconds compile_time_;
151155
hir::Function::InlineFunctionStats inline_function_stats_;
152156
hir::OpcodeCounts hir_opcode_counts_;
157+
// All the deopt patchers pointing to patch points in this function.
158+
std::vector<std::unique_ptr<DeoptPatcher>> deopt_patchers_;
153159
std::unique_ptr<hir::Function> irfunc_;
154160
CodeRuntime* runtime_;
155161
};

cinderx/Jit/compiler.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ std::unique_ptr<CompiledFunction> Compiler::Compile(
184184
}
185185

186186
Timer timer;
187-
std::unique_ptr<jit::hir::Function> irfunc(jit::hir::buildHIR(preloader));
187+
std::unique_ptr<hir::Function> irfunc(hir::buildHIR(preloader));
188188
if (nullptr != compilation_phase_timer) {
189189
compilation_phase_timer->end();
190190
}
@@ -253,6 +253,7 @@ std::unique_ptr<CompiledFunction> Compiler::Compile(
253253
hir_opcode_counts,
254254
code_runtime);
255255
compiled_func->setCompileTime(compile_time);
256+
compiled_func->setDeoptPatchers(std::move(irfunc->deopt_patchers));
256257
if (getConfig().log.debug) {
257258
irfunc->setCompilationPhaseTimer(nullptr);
258259
compiled_func->setHirFunc(std::move(irfunc));

cinderx/Jit/hir/hir.h

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4408,9 +4408,16 @@ class Function {
44084408

44094409
Environment env;
44104410

4411-
// optional property used to track time taken for individual compilation
4411+
// All the deopt patchers pointing to patch points in this function.
4412+
//
4413+
// These will be moved over to the CompiledFunction after compilation is
4414+
// complete.
4415+
std::vector<std::unique_ptr<DeoptPatcher>> deopt_patchers;
4416+
4417+
// Optional property used to track time taken for individual compilation
44124418
// phases
4413-
std::unique_ptr<CompilationPhaseTimer> compilation_phase_timer{nullptr};
4419+
std::unique_ptr<CompilationPhaseTimer> compilation_phase_timer;
4420+
44144421
// Return the total number of arguments (positional + kwonly + varargs +
44154422
// varkeywords)
44164423
int numArgs() const;
@@ -4440,6 +4447,13 @@ class Function {
44404447

44414448
bool canDeopt() const;
44424449

4450+
template <typename T, typename... Args>
4451+
T* allocateDeoptPatcher(Args&&... args) {
4452+
deopt_patchers.emplace_back(
4453+
std::make_unique<T>(std::forward<Args>(args)...));
4454+
return static_cast<T*>(deopt_patchers.back().get());
4455+
}
4456+
44434457
private:
44444458
DISALLOW_COPY_AND_ASSIGN(Function);
44454459
};

cinderx/Jit/hir/simplify.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -978,8 +978,7 @@ Register* simplifyLoadAttrSplitDict(
978978

979979
Register* receiver = load_attr->GetOperand(0);
980980
auto patchpoint = env.emitInstr<DeoptPatchpoint>(
981-
Runtime::get()->allocateDeoptPatcher<SplitDictDeoptPatcher>(
982-
type, name, keys));
981+
env.func.allocateDeoptPatcher<SplitDictDeoptPatcher>(type, name, keys));
983982
patchpoint->setGuiltyReg(receiver);
984983
patchpoint->setDescr("SplitDictDeoptPatcher");
985984
env.emit<UseType>(receiver, receiver->type());
@@ -1053,7 +1052,7 @@ void emitTypeAttrDeoptPatcher(
10531052
// notifies subtypes of the modified type, so we only have to watch the
10541053
// object's type.
10551054
auto patchpoint = env.emitInstr<DeoptPatchpoint>(
1056-
Runtime::get()->allocateDeoptPatcher<TypeAttrDeoptPatcher>(
1055+
env.func.allocateDeoptPatcher<TypeAttrDeoptPatcher>(
10571056
info.py_type, info.attr_name, info.descr));
10581057
patchpoint->setGuiltyReg(info.receiver);
10591058
patchpoint->setDescr(description);
@@ -1139,7 +1138,7 @@ Register* simplifyLoadAttrGenericDescriptor(Env& env, const DescrInfo& info) {
11391138
// patches on any changes to the type, since type_setattro() calls
11401139
// PyType_Modified() before updating tp_descr_{get,set}.
11411140
auto patchpoint = env.emitInstr<DeoptPatchpoint>(
1142-
Runtime::get()->allocateDeoptPatcher<TypeDeoptPatcher>(descr_type));
1141+
env.func.allocateDeoptPatcher<TypeDeoptPatcher>(descr_type));
11431142
patchpoint->setGuiltyReg(info.receiver);
11441143
patchpoint->setDescr("tp_descr_get/tp_descr_set");
11451144
}

cinderx/Jit/runtime.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,13 +172,6 @@ class Runtime : public IRuntime {
172172
// Release any references this Runtime holds to Python objects.
173173
void releaseReferences();
174174

175-
template <typename T, typename... Args>
176-
T* allocateDeoptPatcher(Args&&... args) {
177-
deopt_patchers_.emplace_back(
178-
std::make_unique<T>(std::forward<Args>(args)...));
179-
return static_cast<T*>(deopt_patchers_.back().get());
180-
}
181-
182175
LoadAttrCache* allocateLoadAttrCache() {
183176
return load_attr_caches_.allocate();
184177
}
@@ -312,7 +305,6 @@ class Runtime : public IRuntime {
312305

313306
// References to Python objects held by this Runtime
314307
std::unordered_set<ThreadedRef<PyObject>> references_;
315-
std::vector<std::unique_ptr<DeoptPatcher>> deopt_patchers_;
316308
Builtins builtins_;
317309

318310
std::unordered_map<BorrowedRef<PyTypeObject>, std::vector<TypeDeoptPatcher*>>

cinderx/RuntimeTests/deopt_patcher_test.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,7 @@ def func():
9191
ASSERT_TRUE(term->IsReturn()) << *term;
9292

9393
// Insert a patchpoint immediately before the return
94-
jit::Runtime* jit_rt = jit::Runtime::get();
95-
auto patcher = jit_rt->allocateDeoptPatcher<MyDeoptPatcher>(123);
94+
auto patcher = irfunc->allocateDeoptPatcher<MyDeoptPatcher>(123);
9695
EXPECT_EQ(patcher->id(), 123);
9796
auto patchpoint = jit::hir::DeoptPatchpoint::create(patcher);
9897
patchpoint->InsertBefore(*term);
@@ -107,6 +106,7 @@ def func():
107106

108107
size_t deopts = 0;
109108
auto callback = [&deopts](const jit::DeoptMetadata&) { deopts += 1; };
109+
jit::Runtime* jit_rt = jit::Runtime::get();
110110
jit_rt->setGuardFailureCallback(callback);
111111

112112
// Make sure things work in the nominal case.

0 commit comments

Comments
 (0)