Repository navigation
CppInterOp based cppyy migration - #21261
Conversation
6ddc3ce to
42586b9
Compare
Test Results 24 files 24 suites 4d 0h 38m 22s ⏱️ Results for commit 6071b03. ♻️ This comment has been updated with latest results. |
54c4fb0 to
45f4502
Compare
6d03e1a to
986d7ac
Compare
|
Just rebased, windows builds failed with something related to libafterimage |
986d7ac to
c124bcf
Compare
9336443 to
66eb416
Compare
|
Thanks @aaronj0 for the great progress so far! Just to record where we stand:
|
66fbcd0 to
d3cdb55
Compare
70ef9c3 to
3d9b1de
Compare
|
Great progress! Similar to #21261 (comment), here the summary:
|
3e50982 to
65d8ca1
Compare
|
Update: with the last two [tmp] commits: net roottests: |
ee3eb5c to
b6e1a3f
Compare
|
Update: alma10: 185 (-8) Note: these updates are regression tested, no new failures/early error |
GetType() resolves names that Cpp::GetType() cannot handle by declaring
`using <unique id> = __typeof__(<name>);` in the interpreter. Without a
cache, every call parses and compiles code and adds one more alias to
the AST, growing the interpreter state unboundedly for repeated names.
Memoize the resolved types by name. A name that fails to resolve may
succeed once more declarations are available, so failures are retried.
Upstream motivation: in ROOT, reading a TTree array branch creates the
converter for the branch's element pointer type (e.g. "Double_t*") on
every access, so an event loop went through the trampoline for every
entry. The original reproducer:
import array
import time
import ROOT
n = array.array("i", [3])
e = array.array("d", [1.0, 2.0, 3.0])
tree = ROOT.TTree("tree", "")
tree.Branch("n", n, "n/I")
tree.Branch("e", e, "e[n]/D")
for _ in range(10000):
tree.Fill()
start = time.perf_counter()
for event in tree:
event.e # array branch: the converter for "Double_t*" is created on each access
print(f"loop over 10000 entries reading an array branch: {time.perf_counter() - start:.2f} s")
This takes 0.11 s instead of 1.66 s with this change.
On performance in cppjit proper, the honest result of validating this is
that no equivalent hot path exists here: with an instrumented build, the
full test suite (559 tests) and targeted scenarios (STL container
template instantiation, low-level views, callbacks through
std::function, operator dispatch, exception translation) produce exactly
one trampoline invocation per process, the one-time resolution of
std::exception. Spellings like "double*" are resolved by Cpp::GetType
directly. So there is no cppjit-side number to quote; the cache prevents
AST alias growth for consumers that do drive this path repeatedly, at
the cost of one map lookup per call.
Matches compiler-research/cppjit#117.
Every polymorphic object handed to Python goes through GetActualClass(),
which demangles the dynamic type's name and resolves it with a full
interpreter name lookup, and then through GetBaseOffset(), which walks
the inheritance paths. Neither result was cached, so this cost was paid
again for every returned object.
Memoize both in the interop wrapper, where the calls are already
serialized by the interop lock:
- GetActualClass per (static class, dynamic type_info). Only outcomes
that cannot change are cached: a failed lookup may succeed once more
declarations are available, so it is retried.
- GetBaseOffset per (derived, base) class pair. The object address was
already ignored, so the offset is fixed per pair once both records are
complete. An incomplete record has no walkable bases (the lookup would
read an empty base path), so -- like the error case -- it is treated
silently and retried next time rather than frozen into the cache.
For example, getting objects out of a container of base pointers, with
auto-downcast to the actual derived class on each access:
import time
import cppjit
cppjit.cppdef(r"""
#include <vector>
struct PerfBase {
virtual ~PerfBase() = default;
virtual int value() const { return 1; }
};
struct PerfDerived : PerfBase {
int value() const override { return 2; }
};
std::vector<PerfBase*> make_perf_vec(int n) {
static PerfDerived d[64];
std::vector<PerfBase*> v;
for (int i = 0; i < n; ++i) v.push_back(&d[i % 64]);
return v;
}
""")
vec = cppjit.gbl.make_perf_vec(100)
vec.at(0) # warm-up
start = time.perf_counter()
for _ in range(2000):
for i in range(100):
vec.at(i) # returns PerfBase*, auto-downcast to PerfDerived
print(f"200k vector<PerfBase*>.at() calls: {time.perf_counter() - start:.2f} s")
This takes 0.18 s instead of 0.40 s per 200k accesses (Release build
against the pinned CppInterOp, min of 7 repetitions, interleaved A/B
against the pristine library, ~2.3x). An instrumented build confirms one
GetActualClass() and one GetBaseOffset() call per element access before
this change.
If the name lookup is expensive -- the actual class is templated with a
non-type template argument, e.g. `Impl<float, (EOp)0>`, whose spelled
name needs interpreter-assisted resolution -- the difference is much
larger: a single returned base pointer cost ~420 us and now costs
~0.6 us.
Matches compiler-research/cppjit#115.
Passing an object of a derived class to a formal base-class argument, as
well as binding a method call whose 'self' is an instance of a class
derived from the method's declaring class, makes the bindings check
interop::IsSubclass() on every call, and the clang derivation check is
not free.
Memoize IsSubclass() per class pair in the interop wrapper, where the
calls are already serialized by the interop lock. A class that is still
incomplete can gain bases once its definition is loaded, so a negative
answer is only cached when both classes are complete.
For example, calling a C++ function that takes a base-class reference
with a derived instance:
import time
import cppjit
cppjit.cppdef(r"""
struct SubBase {
virtual ~SubBase() = default;
virtual int value() const { return 1; }
};
struct SubDerived : SubBase {
int value() const override { return 2; }
};
int take_base(const SubBase& b) { return b.value(); }
""")
d = cppjit.gbl.SubDerived()
cppjit.gbl.take_base(d) # warm-up
start = time.perf_counter()
for _ in range(200000):
cppjit.gbl.take_base(d)
print(f"200k calls: {time.perf_counter() - start:.2f} s")
This takes 0.08 s instead of 0.22 s per 200k calls in a pristine build
(0.18 s with only the parent commit's caches applied; ~2x further).
Release build against the pinned CppInterOp, min of 7 repetitions,
interleaved A/B. An instrumented build confirms the converter queries
IsSubclass() once per call on this path, so the saved cost is the clang
derivation check itself.
Matches compiler-research/cppjit#116.
|
CMSSW report: cms-sw#236 (comment) |
dpiparo
left a comment
dpiparo
left a comment
There was a problem hiding this comment.
Congratulations, @aaronj0, for all your high-quality and tireless work: you were the driving force of the new Python-C++ interoperability approach, for the benefit of our community. This upgrade is a substantial step towards ROOT 7, already in ROOT 6!
I approve these changes and let you schedule the merging at your convenience.
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Thank you very much for this big work indeed!
This is also a big quality of life improvement of us developers: 8k lines of code less, dropping old cppyy features that we didn't want anymore (like pypy) to reduce the maintenance burden. And then of course it's much easier that we have to follow only one cppjit upstream repo now instead of the three old cppyy repos.
There are two unit test failures that are not in cms-sw/cmsdist#10891. Do we have confirmation that these are not caused by the upgrade?
I discussed this, it would be good to put a fork in place. I would also strongly suggest to have something similar for cppjit. |
Modulo my previous comment, we may want to merge with a merge commit, as we do for the LLVM upgrades. It's a bit less "severe" here, but I assume not all tests will pass at all intermediate stages. |
|
About the forks: cppjit is a new package, and we want to clearly establish
On CMSSW: same unit tests failure can also be seen in the ROOT6 integration builds, and the differences in DQM histogram differences are typical for these kind of studies (see cms-sw#235 (comment) for example). About the merge commit: intermediate stages will compile, but not all ROOT Python unit tests pass, because they are sequentially fixed by the following commits. I think this is useful to have in the full history to clearly see the effect of the patches. What would be the advantages of the merge commit here? Non-linear history is usually making things more complicated... |
|
@smuzaffar, you share my assessment on the CMSSW test results? |
Ok, I don't care where they are, but these branches don't seem to be up-to-date with what is proposed in this PR, are they? I would also strongly suggest date-based tags because you will want to force-push these branches going forward. And you need to adapt
Possible, though I don't see anything for
I'm not advocating for a squash. But a merge commit makes it clear that these commits form a unit and caution is necessary if you happen to land on one of the intermediate commits. To keep things managable, we usually close the branch, rebase once more, and then merge with a merge commit (see latest LLVM upgrade). |
yes, cmssw unit tests failure are not related to this change see cms-sw#236 (comment) for details |
The branches weren't synced until approval, the tips now point to exactly what this PR applies (with dated tags). I plan to follow up with updating cppinterop-diff to point to that mirror, and add a "cppjit-diff" job doing the same, not as part of this PR. |
|
Thanks @aaronj0 and @smuzaffar for confirming! The LLVM upgrades are essentially the only precedent for merge commits on master (the other And the justification can't be the broken intermediate-state argument: CI only ever tests PR heads, so no multi-commit PR (long linear series like the webgui work included) certifies anything about its intermediate commits. If non-green intermediates were the criterion, rebase-and-merge could not be our current practice for any multi-commit PR. That said, the bisect hazard you raise is real, but it's just the same one we accept for every other large PR, mitigatable with So I'd rather keep this PR on the standard rebase-and-merge path. If the rule you actually want is "merge commits for some to-be-defined cases", let's have that as a general discussion in a ROOT meeting about what we do with new PRs going forward, so authors get a predictable policy instead of this debate right before each merge. |
|
@aaronj0 From my side this is ready to land: approvals are in, CMS confirmed the unit test failures are unrelated, and the branches/tags are in sync. Thanks for bringing this to the finish line. After the rebase-and-merge, please follow up on the @hahnjo thanks for the review and the scrutiny. Keeping those checks green is the diligence we count on from the codeowner of the interpreter code 🙂 |
In principle, every commit must build and should pass all tests. This should be ensured, especially in multi-commit PRs, otherwise you need to reorganise commits. It's not always possible, LLVM upgrades and this one being the exception (in my opinion).
(I disagree that it is commonly accepted, in my opinion the policy is to not accept this)
The merge commit makes it explicit that you land in such intermediate state 🤷
I don't see what this changes at which point this is discussed, it's just that Danilo needs to hit the merge button... |
FWIW I disagree with merging a broken check. We did this in the beginning of the |
Respectfully, no. It is curtesy of the team who has done the work and @aaronj0 reward for the hard work. Danilo has approved the work so have the other reviewers. @aaronj0, please go ahead, this work cannot be always a victim of rules that apply only partially to a subset of the team. Let’s make this a shared team success rather than a debate about policy. |
Not sure why this was done in separate PRs, in my opinion it would have been more logical to have it here. But thanks for taking care of it.
Ok, though I don't understand why my suggestions, made in good faith and with reasonable technical arguments (that Jonas even agreed with), are dismissed with such passion on the basis of non-technical arguments. FWIW I repeatedly suggested having automated checks with consistent handling of downstream patches, in the last weeks, months, and even years. I also had Aaron agree with me last week, but it seems the approach of forking was overruled for one reason or another. Regarding merging, I don't think a merge commit takes away any credit, the individual commits are still there. And as I said, we've been doing this for the past few years already for the LLVM upgrades... |
|
My comments are also in good faith and I did not assume bad faith on your end either. I am not aware of any side discussions and direction being taken outside of our public design meeting. I am happy to elaborate off list about my reasoning, however, there is no new information since our previous conversations on related topics and probably my arguments will repeat. |
7 participants
Bringing cppjit to ROOT, the successor to the cppyy project, for Python/C++ bindings.
This PR migrates all of ROOT's cppyy codebase to use CppInterOp directly.
The development workflow here is designed to have commits in the following fashion:
A typical update of this PR involves first upgrading CppInterOp in ROOT, independently. Then rebase on top and rework the commits to the above stencil. This allows us to cycle our development periods in the most efficient manner. At the final stage, we will have the new "cppjit" codebase in (See the new layout in https://github.com/compiler-research/cppjit) with the legacy mode turned on to continue supporting the Python
cppyy.gblnamespace.