Remove non-async cluster methods & go full-async#366
Remove non-async cluster methods & go full-async#366gmarcosb wants to merge 9 commits intoproject-chip:mainfrom
Conversation
Methods were already being wrapped in async helpers to translate the method to async, so this removes a lot of code maintenance Rust & LLVM *should* be able to optimize away any async method which don't actually suspend, resulting in similar footprints Allowing all methods to be async simplifies long-running tasks such as writing to disk, scanning networks, etc. In C++ this is currently done via a split - the response message (containing e.g. wifi networks) is sent at a later time via an ad-hock callback Rust having everything be async will provide a consistent & clean interface for all clusters vs hacky callbacks in C++
Summary of ChangesHello @gmarcosb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request transitions the codebase to a fully asynchronous model by removing non-async cluster methods and adapting existing handlers. This change streamlines the interface for long-running tasks and simplifies code maintenance, while leveraging Rust's optimization capabilities to minimize performance impact. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
| pub const fn tlv_element(&self) -> &rs_matter_crate::tlv::TLVElement<'a> { | ||
| &self.0 | ||
| } | ||
| pub fn amount(&self) -> Result<rs_matter_crate::im::Money, rs_matter_crate::error::Error> { |
There was a problem hiding this comment.
@ivmarkov this was weird; unrelated to anything I changed here, almost as though this test wasn't being run in CI? (the change is correct, the type is im:Money not money; just don't understand how this test wasn't failing)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the codebase to a fully asynchronous model by removing non-async cluster methods and the Async wrapper. The changes are consistently applied across various files, including handler definitions, adaptor implementations, and example usages. This refactoring simplifies the API, reduces code maintenance, and aligns with the stated goal of providing a consistent and clean interface for all clusters. The transition to async fn and impl Future return types for delegate methods is well-executed, ensuring that the system now operates uniformly in an asynchronous manner. No issues were found that require specific review comments.
|
PR #366: Size comparison from 9917d2c to 3bc8ec9 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
Well damn, there's a size increase 🤔😭 |
|
PR #366: Size comparison from 9917d2c to ffa5a5a Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
PR #366: Size comparison from 9917d2c to 3f627ce Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
I'm not sure what we should do. On one hand, 5% to 7% code size increase is not exactly small. On the other hand, it is not like 20% or 30% and the change simplifies stuff a lot and I really like it. What do you think? |
|
On the good side, we see 0% RAM increase (right?). |
|
@ivmarkov I'm inclined to agree with you, but if you have any ideas about how we can further optimize (you're the pro, I'm still a noob), I think it'd be worthwhile - is there anything I missed? Any nifty tricks that may trigger the compiler to optimize? In C++, for example, when using templates, the compiler sometimes thinks "I won't inline this, that way it can be efficiently reused!" - but if you force an inline (with Is there anything like that in rust? My gut feel is that the |
Initially I also assumed - for My thinking was as follows:
BUT: now that I'm thinking about this... this optimization cannot work! So, the compiler cannot really optimize and turn let fut /*: Future*/ = foo();... would fail. The compiler must return a "future" over the in-reality-non-awaiting "foo". Even if that future resolves immediately when polled for the first time! So perhaps this ^^^ is the reason for the code bloat we are seeing? Now - and as you say - maybe, if aggressive inlining is turned on (opt-level = 3 BUT we don't want this - we want "optimize for size") and the compiler - prior to LLVM because I don't think LLVM "sees" the async stuff at all - can inline the call to So in theory putting But then we hit the next bump - we have to google because there was a bug that async fn foo() -> u32 { 42 }<=> fn foo() -> impl Future<Output = u32> {
async {
42
}
}I.e. async {
42
}which is your I.e. we don't gain much... because that future needs code, and even if small (because the future resolves right away on the first poll) it would never be just "42". |
|
From what I have read, it can optimize - at the LLVM level Rust cannot optimize obviously because the ABI requires that the method expose a But when LLVM sees that the So, at link time, as long as rust is outputting the correct types which are basically no-ops, LLVM is supposed to be able to see "oh, this returned future is just a value so let me do away with all the logic" That's the theory, anyway Let me try |
Hmmm, but a future that resolves immediately is not a no-op? It is basically something like this (there is a ready type for that in struct MyFuture;
impl Future for MyFuture {
type Output = u32;
fn poll(self: Pin<&mut Self>) -> Poll<Self::Output> {
Poll::Ready(42)
}
}... and then calling /// A lot of extra code above this match
match fut.poll() {
Poll::Ready(value) => value,
Poll::Pending => ...
}So the compiler (or LLVM) will have to inline the call to |
If such optimizations are due ^^^ they are definitely not at link time though? |
|
Sorry, both at link time + compile time; the idea (speaking from C++ experience & translating to rust) is that if at link-time you have something like this (bastardized C++/rust code) Then while linking the library containing At least, that's the theory, if everything is implemented correctly (but I'm honestly not sure if it's link-time or compile-time or both) |
|
It is compile time, and the code that the compiler has to "see through" to figure out it is all just the magic constant "42" seems a bit larger to me than in your example. If you look at my But regardless - apparently this opt does not trigger. :( |
|
BTW opt-level 3 would be a disaster I bet. It aggressively inlines, but this means code size blow up too. opt-level 3 is "optimize for speed", not for size. |
|
PR #366: Size comparison from 9917d2c to ab79ead Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
I might be missing something, but "link time optimizations" are basically "remove dead code". But this is code which never gets called basically. I'm not sure the linker does code optimizations - just dead code removal. And then also a bit - which linker? |
|
50% size increase with |
|
PR #366: Size comparison from 9917d2c to 93e70a9 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
|
gmarcosb
force-pushed
the
new-tests-2
branch
2 times, most recently
from
53f9e62 to
22f25ab
Compare
|
PR #366: Size comparison from 9917d2c to 22f25ab Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
Played around a bit in rust playground... didn't find anything interesting, though, the ASM view for release in the playground does seem to indicate that the Not sure if playground runs through LLVM |
|
Played around with |
gmarcosb
force-pushed
the
new-tests-2
branch
2 times, most recently
from
50050d1 to
1f796c8
Compare
… AsyncClusterHandler This allows for a cluster to easily combine both sync methods + async methods, while reducing the size of the binary
|
PR #366: Size comparison from 9917d2c to 1f796c8 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
Welp, the latest round of using |
|
PR #366: Size comparison from 9917d2c to 528fe92 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
…mize away Ready futures
|
Note from @gmarcosb: This is the size report with my attempted optimizations to force the compiler to see the future as ready & not generate unnecessary PR #366: Size comparison from 9917d2c to 11ab6a1 Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
…s things worse rather than better This reverts commit 11ab6a1.
|
Reverted failed optimization attempt (size report showed an increase); leaving it in the commit history just in case it's useful in the future I've filed with rust to see if this is an optimization they could work on: As it stands, I think this is as optimal as we're going to get The question now is: are we willing to take a 5% size increase hit to make our API flexible for all the endpoints which may be async I think it's worthwhile because:
|
|
PR #366: Size comparison from 9917d2c to c505ffd Increases above 0.2%:
Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
2 participants
Methods were already being wrapped in async helpers to translate the method to async, so this removes a lot of code maintenance
Rust & LLVM should be able to optimize away any async method which don't actually suspend - but sadly does not, filed rust-lang/rust#152141
Allowing all methods to be async simplifies long-running tasks such as writing to disk, scanning networks, etc.
In C++ this is currently done via a split between request message received + response message sent; the response message (containing e.g. wifi networks) is sent at a later time via an ad-hock callback
Rust having everything be async will provide a consistent & clean interface for all clusters vs hacky callbacks in C++