Skip to content

IDL gen improvements 3 - Fold all proc-macro code back into rs-matter-macros#239

Merged
ivmarkov merged 2 commits intoproject-chip:mainfrom
sysgrok:fold-rs-matter-macros
Jun 16, 2025
Merged

IDL gen improvements 3 - Fold all proc-macro code back into rs-matter-macros#239
ivmarkov merged 2 commits intoproject-chip:mainfrom
sysgrok:fold-rs-matter-macros

Conversation

@ivmarkov
Copy link
Contributor

@ivmarkov ivmarkov commented May 22, 2025

(NOTE: #233 and #237 need to go in first or else this PR will show an unrealistically large change-set.)

This PR is addressing #236 in folding all of rs-matter-data-model, rs-matter-data-model-impl and rs-matter-macros into just rs-matter-macros. The original split was done because of (to my recollection) issues with tests in proc-macro crates.

What works in this "folded" setup:

  • Unit tests. I did not even had to move them into a separate tests folder

What does not work:

  • The "example" which is showing how a user can use the IDL parser - without the import! proc-macro - in their code. I don't think that would be a real use case any time soon (if ever) so I just removed this

  • The benchmark. We have a single simple benchmark (in the benches folder) which is benchmarking (a) the IDL parser parsing time, but is not benchmarking (b) the code-generation time, nor it is benchmarking (c) the time it takes rustc to actually compile the code generated by the import! proc-macro (which is relatively large).

I don't have a good answer (not yet at least) how to restore the existing "IDL parser timing" benchmark if we fold all code under the rs-matter-macros crate.

But in any case, I think we need at some point a more extensive benchmark anyway, which is also timing all three aspects (parsing; code generation; time it takes rustc to compile the generated code). As I have a suspicion that the proc-macro execution time might actually be dominated by (c).

@ivmarkov ivmarkov changed the title [Draft, for discussion only] IDL gen improvements 3 - Fold rs matter macros [Draft, for discussion only] IDL gen improvements 3 - Fold all proc-macro code back into rs-matter-macros May 22, 2025
@ivmarkov ivmarkov force-pushed the fold-rs-matter-macros branch 2 times, most recently from 191f046 to 07000cd Compare June 3, 2025 12:29
@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jun 3, 2025

@bjoernQ What do you think about this one? I'm unsure how to proceed.

  • On one hand, the current separation of the proc-macros implementation into non-proc-macro crate(s) do allow more flexibility. It is another topic that we don't need this flexibility for unit tests, as they just work, proc-macro crate or not. We might need this flexibility if we want to keep the current single benchmark that times the IDL parser
  • On the other, having a benchmark that benchmarks all three aspects of the import! macro as per the summary is what we might actually want so as to say that we have a real benchmark, but not sure how to even achieve it. Any ideas?

In any case, my feeling is three proc-macro-related crates is a bit too many, so either everything back into rs-matter-macros (as this PR does), or rs-matter-macros-impl + rs-matter-macros.

What is your feeling?

@bjoernQ
Copy link
Contributor

bjoernQ commented Jun 3, 2025

I probably need to think about this a bit more - so just my initial thoughts here.

My gut feeling is that having all the proc-macro code in just one crate is nicer for everyone. (Not sure how much users would really care but it's at least easier in terms of maintainence)

I recently learnt about https://crates.io/crates/trybuild - I haven't used it yet, but it looks quite interesting and might be something to consider here?

@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jun 3, 2025

I probably need to think about this a bit more - so just my initial thoughts here.

My gut feeling is that having all the proc-macro code in just one crate is nicer for everyone. (Not sure how much users would really care but it's at least easier in terms of maintainence)

I recently learnt about https://crates.io/crates/trybuild - I haven't used it yet, but it looks quite interesting and might be something to consider here?

Let me spend some time with that crate.

@ivmarkov ivmarkov force-pushed the fold-rs-matter-macros branch 6 times, most recently from a9e4512 to eb2a814 Compare June 10, 2025 15:04
@ivmarkov ivmarkov force-pushed the fold-rs-matter-macros branch from eb2a814 to 435c808 Compare June 15, 2025 15:20
@ivmarkov ivmarkov changed the title [Draft, for discussion only] IDL gen improvements 3 - Fold all proc-macro code back into rs-matter-macros IDL gen improvements 3 - Fold all proc-macro code back into rs-matter-macros Jun 16, 2025
@ivmarkov ivmarkov marked this pull request as ready for review June 16, 2025 09:05
@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jun 16, 2025

@bjoernQ I've played with the trybuild macro and I think we should merge this folding back of all code into a single crate.

The TL;DR is:

  • By slightly extending the import! macro, we are able to ask the proc-macro to printout its IDL-parsing time, as well as its codegen time.
  • Further, I implemented the option to put caps on the IDL parsing time, as well as on the codegen-time, which are asserted in a small, trybuild-based test. The test is part of the rs-matter tests, rather than the rs-matter-proc-macro tests, because the generated code is actually compiled, and for that, rs-matter needs to be around, as the proc-macro generated code that uses types from rs-matter.

The IDL is always parsed as one large chunk regardless whether the person is importing a single cluster, or many clusters with a particular import! invocation. While we can think of optimization of that in future, I don't think it is worth it, as parsing time is consistently around 150ms for me, which is acceptable.

The codegen time is proportional to the number of imported clusters. It starts with 11ms for the simplistic OnOff cluster, and baloons to 600+ms if I include all system clusters as well as the UnitTesting cluster behemoth. In any case, not sure how to reduce this time, as it is all just "iterate the in-memory IDL parser AST and generate large code as string" type of job.

There is not yet a timing of the time it takes Rust to compile the generated code. We can add this in future, but it would take some juggling with some extra code on top of trybuild, as we'll have to (a) first invoke the test with trybuild as-is (so that all macro deps are compiled) and (b) then invoke it N times incrementally (perhaps by touch-ing the trybuild test .rs file each time), so that only that file is compiled and we average-out the execution time of rustc.

In any case, my unscientific measurements do prove that the Rust compilation of the code generated by the import! proc-macro is actually dominating the time it takes to invoke the proc-macro, by a very wide margin (seconds). Which goes to say that we should probably not invest any time in optimizing the IDL parser, or the codegen for now.

@ivmarkov ivmarkov requested a review from bjoernQ June 16, 2025 09:16
@ivmarkov ivmarkov force-pushed the fold-rs-matter-macros branch from 68542fd to 2a20a25 Compare June 16, 2025 10:26
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Getting rid of the two additional crates is a nice step forward 👍

@ivmarkov ivmarkov merged commit 4fef5f2 into project-chip:main Jun 16, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants