genbindings: Streaming parse clang output to reduce memory, get correct filepaths#226
genbindings: Streaming parse clang output to reduce memory, get correct filepaths#226mappu merged 5 commits intomappu:masterfrom
Conversation
|
draft because of the numbered renames - need some help with these in case we want to merge this |
To prepare genbindings for accepting arbitrary libraries from the command line, we must retain parse types not only from the header being processed but also its includes, to understand things like type hierarchies etc without depending on a particular module order. This is the first baby step that addresses clang AST filtering. The change has some collateral benefits: the improved accuracy of the AST parsing keeps more relevant information but at the same time reduces the memory footprint since filtering is done streaming instead of loading everything into memory first (ditto when writing the cache). A consequence is that a lot more clang processes can run in parallel without OOMing. * parse "file" correctly to discover type provenance - using the standard go json parser for this does not work since the depend on serialization order which go discards (see https://github.com/dtolnay/clang-ast?tab=readme-ov-file#source-locations) * stream clang output to JSON decoder and stream-write the cache to reduce memory footprint * with the newfound memory efficiency, bump up the number of parallel jobs and use threads for the parsing as well * fix missing classes such as QSysInfo using the corrected `file` source location field * don't try to re-run clang if it fails (OOM is unlikely anyway)
arnetheduck
force-pushed
the
miqt-ast-full
branch
from
d564893 to
6de4e76
Compare
| #endif | ||
|
|
||
| QQmlListReference* QQmlListReference_new(); | ||
| QQmlListReference* QQmlListReference_new2(QVariant* variant); |
There was a problem hiding this comment.
the interesting thing about these removed overloads is that 3 and 4 are the same .. 🤷
|
the qmllist changes look legit as far as I can tell, so undrafting |
|
ping @mappu anything more needed here? |
|
I think this LGTM 🚢 The change makes sense to me, just need a spare evening to test out the performance delta on my PC. I really wish there was a cleaner way of doing the JSON munging but at least it's pretty well isolated. |
|
Do we want to assess the conflicts this will have with #118? The thought of rebasing somewhat terrifies me. I don't know that I'll be applying this change downstream either. |
Semantically, I believe there is no conflict - this PR basically enriches the incoming JSON from clang with correct provenance information - from that point of view, the two pr:s address different stages of the processing pipeline, this PR fixing the input to what #118 has to transform - in a way, this PR simply reduces the garbage coming into #118 and later stages and thus reduces the garbage coming out of them. There might indeed be some minor code adjustments to make, though these should in principle be trivial since it's still the same data being processed (just that it's now correct:er) |
Handling |
|
Thank you... and I hate to be a nag but this PR will likely not be squashed and merged which means rebasing still means processing each commit from the PR... meaning this adds to the rebase effort. Rather than nagging like this, would it be easier if we just got the other PR sorted first? Is there any rush on this being merged? |
|
I don't mind either way, from the test merge I did locally of the two, there were only a few small adjustments regardless of the order. Conflicts happen, it's a fact of life in async development ;) I have a few other PR:s lined up that could also maybe benefit miqt that depend on this one, so it's more a matter that it's holding those up as well and since yours is still in draft, I didn't want to wait indefinitely before posting this one. |
That's on me. I've been too excited which led to me being sloppy and the draft is to prevent an accidental merge of something that I know needs changes. I need to rebase with the fixes for the iterator types and then figure out something cleaner for Qt 5 and then there's the quirk of the private signals disappearing. But I think it's important enough to land since it can unblock or trivialize supporting Qt 6.8 (and more?) as a base. |
|
I ran some genbindings benchmarks on this branch: v0.11.2:
I did some small tests and a single I think i'd like to merge this PR for now. The correctness issue with 'file' gets us onto much better footing. We can help resolve conflicts with the other branch, |
mappu
changed the title
I'll take a shot at rebasing either this weekend or the next. I've had to touch up the implementation anyways but it's been stress tested quite a bit by now as well. The quirks that I remember are finding a way to fence off Qt 5 from the changes and the private signals disappearing on Qt 6. @mappu Would you rather see the PR for 6.8 resolved first or upstreaming support for FunctionDecl and FieldDecl? I still need to read through the reimplementation from this PR but the patches for those Decls from downstream should hopefully still carry over. |
|
I think FunctionDecl/FieldDecl are the more important changes, as they're more user facing. But either is fine (whatever makes your life easier!) |
Nah, there's just more data to parse, in general, because each cached file now contains more data - basically, the full translation unit (including After this PR it's possible to further refactor the code to process each header file out-of-order, ie all the globals in the generator can be turned to locals. In another language, one would probably use a "cache"/work area per thread to save on the analysis, not sure how that's done in golang since you don't control the threads - probably a "cache pool" of some sort where each "job" checks out a cache and hands it back when it's done. With that out of the way, the header processing itself can be paralelised, but more importantly, you can run it on any one single header file and get a deterministic result. |
3 participants
To prepare genbindings for accepting arbitrary libraries from the command line, we must retain parse types not only from the header being processed but also its includes, to understand things like type hierarchies etc without depending on a particular module order.
This is the first baby step that addresses clang AST filtering.
The change has some collateral benefits: the improved accuracy of the AST parsing keeps more relevant information but at the same time reduces the memory footprint since filtering is done streaming instead of loading everything into memory first (ditto when writing the cache).
A consequence is that a lot more clang processes can run in parallel without OOMing.
https://github.com/dtolnay/clang-ast?tab=readme-ov-file#source-locations)
filesource location field