Skip to content

Commit

Permalink
Fix circular dependencies between GenDataFooter and GenYieldPoint
Browse files Browse the repository at this point in the history 
Summary:
GenDataFooter contains a GenYieldPoint*, which is the simpler class.  However
GenYieldPoint had methods that took a GenDataFooter, so the two could not be
defined without forward declarations.  GenYieldPoint also had methods that would
load the Runtime singleton under the hood, which encourages more of these kinds
of circular dependencies.

This diff unravels some of the methods used by GenYieldPoint, moves more of the
responsibility onto callers to access data via GenDataFooter and Runtime
directly.  The methods each only had one callsite, and they were all in
pyjit.cpp where it makes much more sense to load the Runtime singleton.  There
is a now a simpler dependency graph from:

  GenYieldPoint -> GenDataFooter -> CodeRuntime -> Runtime

Reviewed By: jbower-fb

Differential Revision: D55761655

fbshipit-source-id: 1a26568ae2eef715929b12c5dae207a69860b69e
  • Loading branch information
@facebook-github-bot
Alex Malyshev authored and facebook-github-bot committed Apr 5, 2024
1 parent d43fc11 commit 3aa5e13
Show file tree
Hide file tree
Showing 5 changed files with 132 additions and 134 deletions.
2 changes: 0 additions & 2 deletions cinderx/Jit/deopt_patcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@

namespace jit {

class CodeRuntime;

// A DeoptPatcher is used by the runtime to invalidate compiled code when an
// invariant that the compiled code relies on is invalidated. It is intended
// to be used in conjuction with the DeoptPatchpoint HIR instruction.
Expand Down
2 changes: 0 additions & 2 deletions cinderx/Jit/frame.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@

namespace jit {

class CodeRuntime;

// FrameHeader lives at the beginning of the stack frame for JIT-compiled
// functions. Note these will be garbage in generator objects.
struct FrameHeader {
Expand Down
40 changes: 25 additions & 15 deletions cinderx/Jit/pyjit.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1516,7 +1516,7 @@ static PyObject* disable_hir_inliner(PyObject* /* self */, PyObject*) {
// If the given generator-like object is a suspended JIT generator, deopt it
// and return 1. Otherwise, return 0.
static int deopt_gen_impl(PyGenObject* gen) {
auto footer = reinterpret_cast<GenDataFooter*>(gen->gi_jit_data);
GenDataFooter* footer = genDataFooter(gen);
if (Ci_GenIsCompleted(gen) || footer == nullptr) {
return 0;
}
Expand Down Expand Up @@ -2350,7 +2350,7 @@ PyObject* _PyJIT_GenSend(
PyFrameObject* f,
PyThreadState* tstate,
int finish_yield_from) {
auto gen_footer = reinterpret_cast<GenDataFooter*>(gen->gi_jit_data);
GenDataFooter* gen_footer = genDataFooter(gen);

// state should be valid and the generator should not be completed
JIT_DCHECK(
Expand Down Expand Up @@ -2407,33 +2407,43 @@ PyFrameObject* _PyJIT_GenMaterializeFrame(PyGenObject* gen) {
}

int _PyJIT_GenVisitRefs(PyGenObject* gen, visitproc visit, void* arg) {
auto gen_footer = reinterpret_cast<GenDataFooter*>(gen->gi_jit_data);
GenDataFooter* gen_footer = genDataFooter(gen);
JIT_DCHECK(gen_footer, "Generator missing JIT data");
if (gen_footer->state != Ci_JITGenState_Completed && gen_footer->yieldPoint) {
return reinterpret_cast<GenYieldPoint*>(gen_footer->yieldPoint)
->visitRefs(gen, visit, arg);
const GenYieldPoint* yield_point = gen_footer->yieldPoint;
if (gen_footer->state != Ci_JITGenState_Completed && yield_point) {
size_t deopt_idx = yield_point->deoptIdx();
return Runtime::get()->forEachOwnedRef(gen, deopt_idx, [&](PyObject* v) {
Py_VISIT(v);
return 0;
});
}
return 0;
}

void _PyJIT_GenDealloc(PyGenObject* gen) {
auto gen_footer = reinterpret_cast<GenDataFooter*>(gen->gi_jit_data);
GenDataFooter* gen_footer = genDataFooter(gen);
JIT_DCHECK(gen_footer, "Generator missing JIT data");
if (gen_footer->state != Ci_JITGenState_Completed && gen_footer->yieldPoint) {
reinterpret_cast<GenYieldPoint*>(gen_footer->yieldPoint)->releaseRefs(gen);
const GenYieldPoint* yield_point = gen_footer->yieldPoint;
if (gen_footer->state != Ci_JITGenState_Completed && yield_point) {
size_t deopt_idx = yield_point->deoptIdx();
Runtime::get()->forEachOwnedRef(gen, deopt_idx, [](PyObject* v) {
Py_DECREF(v);
return 0;
});
}
JITRT_GenJitDataFree(gen);
}

PyObject* _PyJIT_GenYieldFromValue(PyGenObject* gen) {
auto gen_footer = reinterpret_cast<GenDataFooter*>(gen->gi_jit_data);
GenDataFooter* gen_footer = genDataFooter(gen);
JIT_DCHECK(gen_footer, "Generator missing JIT data");
PyObject* yf = nullptr;
if (gen_footer->state != Ci_JITGenState_Completed && gen_footer->yieldPoint) {
yf = gen_footer->yieldPoint->yieldFromValue(gen_footer);
Py_XINCREF(yf);
const GenYieldPoint* yield_point = gen_footer->yieldPoint;
PyObject* yield_from = nullptr;
if (gen_footer->state != Ci_JITGenState_Completed && yield_point) {
yield_from = yieldFromValue(gen_footer, yield_point);
Py_XINCREF(yield_from);
}
return yf;
return yield_from;
}

PyObject* _PyJIT_GetGlobals(PyThreadState* tstate) {
Expand Down
46 changes: 0 additions & 46 deletions cinderx/Jit/runtime.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,44 +16,6 @@ namespace jit {
const int64_t CodeRuntime::kPyCodeOffset =
RuntimeFrameState::codeOffset() + CodeRuntime::frameStateOffset();

namespace {
template <typename F>
REQUIRES_CALLABLE(F, int, PyObject*)
int forEachOwnedRef(PyGenObject* gen, std::size_t deopt_idx, F func) {
const DeoptMetadata& meta = Runtime::get()->getDeoptMetadata(deopt_idx);
auto base = reinterpret_cast<char*>(gen->gi_jit_data);
for (const LiveValue& value : meta.live_values) {
if (value.ref_kind != hir::RefKind::kOwned) {
continue;
}
codegen::PhyLocation loc = value.location;
JIT_CHECK(
!loc.is_register(),
"DeoptMetadata for Yields should not reference registers");
int ret = func(*reinterpret_cast<PyObject**>(base + loc.loc));
if (ret != 0) {
return ret;
}
}
return 0;
}
} // namespace

int GenYieldPoint::visitRefs(PyGenObject* gen, visitproc visit, void* arg)
const {
return forEachOwnedRef(gen, deopt_idx_, [&](PyObject* v) {
Py_VISIT(v);
return 0;
});
}

void GenYieldPoint::releaseRefs(PyGenObject* gen) const {
forEachOwnedRef(gen, deopt_idx_, [](PyObject* v) {
Py_DECREF(v);
return 0;
});
}

void CodeRuntime::releaseReferences() {
references_.clear();
}
Expand All @@ -69,14 +31,6 @@ void CodeRuntime::addReference(BorrowedRef<> obj) {
return addReference(Ref<>::create(obj));
}

PyObject* GenYieldPoint::yieldFromValue(GenDataFooter* gen_footer) const {
if (!isYieldFrom_) {
return nullptr;
}
return reinterpret_cast<PyObject*>(
*(reinterpret_cast<uint64_t*>(gen_footer) + yieldFromOffs_));
}

void Builtins::init() {
ThreadedCompileSerialize guard;
if (is_initialized_) {
Expand Down
176 changes: 107 additions & 69 deletions cinderx/Jit/runtime.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,74 +26,8 @@

namespace jit {

class GenYieldPoint;
class TypeDeoptPatcher;

// In a regular JIT function spill-data is stored at negative offsets from RBP
// and RBP points into the system stack. In JIT generators spilled data is still
// stored backwards from RBP, but RBP points to a heap allocated block and this
// persists when the generator is suspended.
//
// While the content of spill data is arbitrary depending on the function, we
// also have a few items of data about the current generator we want to access
// quickly. We can do this via positive offsets from RBP into the
// GenDataFooter struct defined below.
//
// Together the spill data and GenDataFooter make up the complete JIT-specific
// data needed for a generator. PyGenObject::gi_jit_data points above the _top_
// of the spill data (i.e. at the start of the footer). This allows us to
// easily set RBP to the pointer value on generator resume.
//
// The base address of the complete heap allocated suspend data is:
// PyGenObject::gi_jit_data - GenDataFooter::spillWords
typedef struct _GenDataFooter {
// Tools which examine/walk the stack expect the following two values to be
// ahead of RBP.
uint64_t linkAddress;
uint64_t returnAddress;

// RBP that was swapped out to point to this spill-data.
uint64_t originalRbp;

// Static data specific to the current yield point. Only non-null when we are
// suspended.
GenYieldPoint* yieldPoint;

// Current overall state of the JIT.
CiJITGenState state;

// Allocated space before this struct in 64-bit words.
size_t spillWords;

// Entry-point to resume a JIT generator.
GenResumeFunc resumeEntry;

// Associated generator object
PyGenObject* gen;

// JIT metadata for associated code object
CodeRuntime* code_rt{nullptr};
} GenDataFooter;

// These fields need to be at a fixed offset so they can be quickly accessed
// from C code.
static_assert(
offsetof(GenDataFooter, state) == Ci_GEN_JIT_DATA_OFFSET_STATE,
"Byte offset for state shifted");
static_assert(
offsetof(GenDataFooter, yieldPoint) == Ci_GEN_JIT_DATA_OFFSET_YIELD_POINT,
"Byte offset for yieldPoint shifted");

// The number of words for pre-allocated blocks in the generator suspend data
// free-list. I chose this based on it covering 99% of the JIT generator
// spill-sizes needed when running 'make testcinder_jit' at the time I collected
// this data. For reference:
// 99.9% coverage came at 256 spill size
// 99.99% was at 1552
// max was 4999
// There were about ~15k JIT generators in total during the run.
const size_t kMinGenSpillWords = 89;

class GenYieldPoint {
public:
explicit GenYieldPoint(
Expand All @@ -116,9 +50,13 @@ class GenYieldPoint {
return deopt_idx_;
}

int visitRefs(PyGenObject* gen, visitproc visit, void* arg) const;
void releaseRefs(PyGenObject* gen) const;
PyObject* yieldFromValue(GenDataFooter* gen_footer) const;
bool isYieldFrom() const {
return isYieldFrom_;
}

ptrdiff_t yieldFromOffset() const {
return yieldFromOffs_;
}

static constexpr int resumeTargetOffset() {
return offsetof(GenYieldPoint, resume_target_);
Expand Down Expand Up @@ -262,6 +200,85 @@ class alignas(16) CodeRuntime {
DebugInfo debug_info_;
};

// In a regular JIT function spill-data is stored at negative offsets from RBP
// and RBP points into the system stack. In JIT generators spilled data is still
// stored backwards from RBP, but RBP points to a heap allocated block and this
// persists when the generator is suspended.
//
// While the content of spill data is arbitrary depending on the function, we
// also have a few items of data about the current generator we want to access
// quickly. We can do this via positive offsets from RBP into the
// GenDataFooter struct defined below.
//
// Together the spill data and GenDataFooter make up the complete JIT-specific
// data needed for a generator. PyGenObject::gi_jit_data points above the _top_
// of the spill data (i.e. at the start of the footer). This allows us to
// easily set RBP to the pointer value on generator resume.
//
// The base address of the complete heap allocated suspend data is:
// PyGenObject::gi_jit_data - GenDataFooter::spillWords
struct GenDataFooter {
// Tools which examine/walk the stack expect the following two values to be
// ahead of RBP.
uint64_t linkAddress;
uint64_t returnAddress;

// RBP that was swapped out to point to this spill-data.
uint64_t originalRbp;

// Static data specific to the current yield point. Only non-null when we are
// suspended.
GenYieldPoint* yieldPoint;

// Current overall state of the JIT.
CiJITGenState state;

// Allocated space before this struct in 64-bit words.
size_t spillWords;

// Entry-point to resume a JIT generator.
GenResumeFunc resumeEntry;

// Associated generator object
PyGenObject* gen;

// JIT metadata for associated code object
CodeRuntime* code_rt{nullptr};
};

inline GenDataFooter* genDataFooter(PyGenObject* gen) {
return reinterpret_cast<GenDataFooter*>(gen->gi_jit_data);
}

inline PyObject* yieldFromValue(
GenDataFooter* gen_footer,
const GenYieldPoint* yield_point) {
return yield_point->isYieldFrom()
? reinterpret_cast<PyObject*>(
*(reinterpret_cast<uint64_t*>(gen_footer) +
yield_point->yieldFromOffset()))
: nullptr;
}

// These fields need to be at a fixed offset so they can be quickly accessed
// from C code.
static_assert(
offsetof(GenDataFooter, state) == Ci_GEN_JIT_DATA_OFFSET_STATE,
"Byte offset for state shifted");
static_assert(
offsetof(GenDataFooter, yieldPoint) == Ci_GEN_JIT_DATA_OFFSET_YIELD_POINT,
"Byte offset for yieldPoint shifted");

// The number of words for pre-allocated blocks in the generator suspend data
// free-list. I chose this based on it covering 99% of the JIT generator
// spill-sizes needed when running 'make testcinder_jit' at the time I collected
// this data. For reference:
// 99.9% coverage came at 256 spill size
// 99.99% was at 1552
// max was 4999
// There were about ~15k JIT generators in total during the run.
constexpr size_t kMinGenSpillWords = 89;

// Information about the runtime behavior of a single deopt point: how often
// it's been hit, and the frequency of guilty types, if applicable.
struct DeoptStat {
Expand Down Expand Up @@ -445,6 +462,27 @@ class Runtime {
BorrowedRef<PyTypeObject> lookup_type,
BorrowedRef<PyTypeObject> new_type);

template <typename F>
REQUIRES_CALLABLE(F, int, PyObject*)
int forEachOwnedRef(PyGenObject* gen, std::size_t deopt_idx, F func) {
const DeoptMetadata& meta = getDeoptMetadata(deopt_idx);
auto base = reinterpret_cast<char*>(genDataFooter(gen));
for (const LiveValue& value : meta.live_values) {
if (value.ref_kind != hir::RefKind::kOwned) {
continue;
}
codegen::PhyLocation loc = value.location;
JIT_CHECK(
!loc.is_register(),
"DeoptMetadata for Yields should not reference registers");
int ret = func(*reinterpret_cast<PyObject**>(base + loc.loc));
if (ret != 0) {
return ret;
}
}
return 0;
}

private:
static Runtime* s_runtime_;

Expand Down

0 comments on commit 3aa5e13

Please sign in to comment.