yew-macro: optimise macros' memory usage
#3845
Conversation
Size Comparison
✅ None of the examples has changed their size significantly. |
|
Visit the preview URL for this PR (updated for commit 3c7824d): https://yew-rs-api--pr3845-remove-to-string-cal-r1zmbvp8.web.app (expires Sat, 10 May 2025 16:44:00 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - SSRYew Master
Pull Request
|
its-the-shrimp
force-pushed
the
remove_to_string_calls_from_yew_macro
branch
3 times, most recently
from
788cc44 to
40e4caa
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes macro memory usage in the yew-macro crate by eliminating many unnecessary allocations using the new DisplayExt trait methods. Key changes include:
- Replacing .to_string() comparisons with non-allocating DisplayExt methods (repr_eq, repr_eq_ignore_ascii_case, etc.).
- Removing the custom non_capitalized_ascii helper in favor of a method on Ident and similar types.
- Updating diff files across props, html_tree, hook, and function component modules to use the new DisplayExt APIs.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/yew-macro/src/props/prop_macro.rs | Added import for DisplayExt and updated associated properties checking logic. |
| packages/yew-macro/src/props/prop.rs | Replaced .to_string() comparisons with DisplayExt's repr_eq methods. |
| packages/yew-macro/src/lib.rs | Removed non_capitalized_ascii and added DisplayExt trait implementation. |
| packages/yew-macro/src/html_tree/tag.rs | Updated error replacement to pass the original error instead of its string version. |
| packages/yew-macro/src/html_tree/mod.rs | Simplified imports and replaced non_capitalized_ascii with DisplayExt methods. |
| packages/yew-macro/src/html_tree/lint/mod.rs | Updated attribute comparisons using repr_eq_ignore_ascii_case. |
| packages/yew-macro/src/html_tree/html_node.rs | Updated logic to use DisplayExt for literal comparisons. |
| packages/yew-macro/src/html_tree/html_element.rs | Replaced string comparisons with DisplayExt methods and cleaned up related logic. |
| packages/yew-macro/src/html_tree/html_dashed_name.rs | Removed redundant eq_ignore_ascii_case helper in favor of DisplayExt-based checks. |
| packages/yew-macro/src/html_tree/html_component.rs | Added is_component_name helper using DisplayExt, and updated component name logic. |
| packages/yew-macro/src/hook/mod.rs | Updated hook naming checks to use starts_with directly on Identifier. |
| packages/yew-macro/src/hook/body.rs | Replaced to_string() checks with non-allocating starts_with comparisons. |
| packages/yew-macro/src/function_component.rs | Updated attribute filtering using DisplayExt methods for consistency. |
Comments suppressed due to low confidence (1)
packages/yew-macro/src/lib.rs:88
- Consider adding unit tests for the new DisplayExt methods to verify their correctness and prevent regressions.
trait DisplayExt: Display {
There was a problem hiding this comment.
If you've done any measurement on performance impact, feel free to add this to the PR, but since you already did the work, I still nevertheless think it's a nice idea that can serve some purpose. (in general more performance is left on the table in yew-macro by quoting, then parsing and quoting again into different token streams but that's a different issue).
Our CI doesn't measure anything inside the macro or during compilation, only of the resulting binaries, so will be a bit useless here for performance.
| is_ide_completion: is_ide_completion(), | ||
| empty: true, | ||
| }; | ||
| write!(writer, "{s}").is_ok_and(|_| !writer.empty) |
There was a problem hiding this comment.
This deserves a better comment and improved naming, as it is very hard to follow currently.
A name is, at the moment, assumed to be a component name if it
- starts with an ascii uppercase letter
- OR ide completion is enable and it contains any ascii uppercase
Both of these are rough approximations, for example I think the second condition could be changed to match a specific identifier passed via the RUST_IDE_PROC_MACRO_COMPLETION_DUMMY_IDENTIFIER env variable, and I'm not even sure how much adoption that experiment got at this point.
packages/yew-macro/src/lib.rs
Outdated
| /// Needed to check the plentiful token-like values in the impl of the macros, which are | ||
| /// `Display`able but which either correspond to multiple source code tokens, or are themselves | ||
| /// tokens that don't provide a reference to their repr. | ||
| trait DisplayExt: Display { |
There was a problem hiding this comment.
I like the idea! But alas, the current implementation is hard to read when you first see it.
Let me phrase the idea differently. We use write! to feed chunks to an accumulator instead of writing them into a new allocation via to_string. This could be more clear, in my opinion, if the implementations would go through a common method expressing this.
/// A pattern can consume successive chunks of a String to determine
// if it matches, until the end of string is reached.
trait Pattern {
type Match; // bool for all in this PR. Could one imagine more uses?
// Returns a Result to enable short-curcuit. Another idea would be std::ops::ControlFlow<Self::Match>
fn feed_chunk(&mut self, chunk: &str) -> Result<(), Self::Match>;
fn end_of_string(self) -> Self::Match;
}
// Or add this as a method to DisplayExt
fn match_repr<P: Pattern>(this: impl Display, pattern: P) -> P::Match {
struct PatternWrite<P: Pattern> { pattern: P, r#match: Option<P::Match> }
impl<P: Pattern> Write for PatternWrite<P> {
fn write_str(&mut self, chunk: &str) -> std::fmt::Result {
debug_assert!(self.r#match.is_none());
self.pattern.feed_chunk(chunk).map_err(|m| {
self.r#match = Some(m);
std::fmt::Error // Short-curcuit further chunks
})
}
}
let mut writer = PatternWrite { pattern, r#match: None };
match write!(writer, "{this}") {
Ok(()) => writer.pattern.end_of_string(),
Err(_) => writer.r#match.unwrap(),
}
}With this machinery in place, I think a lot of the checks become a lot more digestible. For example
fn split_off_start<'s>(s: &mut &'s str, mid: usize) -> Option<&'s str> {
if mid <= s.len() {
let start;
(start, *s) = s.split_at(mid);
Some(start)
} else {
None
}
}
struct EqualsIgnoreAsciiCase<'src>(&'src str);
impl Pattern for EqualsIgnoreAsciiCase<'_> {
type Match = bool;
fn feed_chunk(&mut self, chunk: &str) -> Result<(), bool> {
match split_off_start(&mut self.0, chunk.len()) {
Some(start) if start.eq_ignore_ascii_case(chunk) => Ok(()),
_ => Err(false),
}
}
fn end_of_string(self) -> bool {
self.0.is_empty()
}
}
struct EqualsExact<'src>(&'src str);
impl Pattern for EqualsExact<'_> {
type Match = bool;
fn feed_chunk(&mut self, chunk: &str) -> Result<(), bool> {
match split_off_start(&mut self.0, chunk.len()) {
Some(start) if chunk == start => Ok(()),
_ => Err(false),
}
}
fn end_of_string(self) -> bool {
self.0.is_empty()
}
}
trait DisplayExt: Display {
fn repr_eq_ignore_ascii_case(&self, pattern: &str) -> bool {
match_repr(self, EqualsIgnoreAsciiCase(pattern))
}
// Etc...
}There was a problem hiding this comment.
Having types & traits that exist solely to implement other traits seems like overengineering that wouldn't be easier to manage, I think with a good explanation & testing, my admittedly cryptic impl will be manageable
There was a problem hiding this comment.
The main point is that the trait and function would translate between two different semantic domains that is worth the overhead.
Write returns a very generic std::fmt::Error "pretending" it failed. At a different point in the code (what follows write!() basically) that error is translated back into the intended result of returning early. So basically when reviewing I had to go jump from inside the write_str to some other point in the code and do that translation in my head. Writing it out explicitly makes it just so much easier to understand what's going on.
At the same time, the part after the write!() also handles the path when no early return happened and we had to look at all the chunks.
There was a problem hiding this comment.
But your suggested impl still has to use the overly generic std::fmt::Error, is still broken up into multiple parts, but with extra concepts on top, so the reader has to keep more in the head, not less. I think this algorithm will be hard to read anyway, because it has to account for chunked input
There was a problem hiding this comment.
The different impls of Pattern do not have to keep this in mind, only the single instance of the glue code in match_repr has to.
There was a problem hiding this comment.
But they still do, your suggested Pattern trait just renames write_str to feed_chunk, but the difficulty of handling chunked input is still there
There was a problem hiding this comment.
Renaming is not the part that makes it meaningful, changing the return type from Result<(), std::fmt::Error> to ControlFlow<bool> (the more appropriate type than Result the more I think about it) is. This is only 1-to-1 if the impl either only short-curcuits on true or false, not possible either. Feel free to bikeshed over the name of the method or just keep write_str if that doesn't lead to ambiguities.
Just because the transformation is simple doesn't mean it's not meaningful, and I still think the dozen lines of glue code make every impl much more approachable.
There was a problem hiding this comment.
@Madoshakalaka feel free to chime in. Maybe i'm too critical or not smart enough to read the current code :p
its-the-shrimp
force-pushed
the
remove_to_string_calls_from_yew_macro
branch
from
40e4caa to
0e8a30a
Compare
|
I also snuck in 1 change from |
its-the-shrimp
force-pushed
the
remove_to_string_calls_from_yew_macro
branch
2 times, most recently
from
913135b to
e5cc234
Compare
- Removed most of `.to_string()` calls
- Factored out the check for a component-like name
its-the-shrimp
force-pushed
the
remove_to_string_calls_from_yew_macro
branch
from
e5cc234 to
3c7824d
Compare
|
Documented the impl & added tests |
|
Additional tests look good, thanks. |
|
I did some memory benchmarks on the I picked the crate due to its most intensive usage of the macros: I tested the memory consumption of psrecord --interval 0.1 --plot memory.png --log memory.csv --include-children -- 'cargo build'memory.png looks like this:
Then I used this to calculate the average memory consumption and the area under the memory consumption: awk 'NR > 1 && $0 !~ /^#/ { sum += $3 } END {
n = NR - 1
print "Average memory (MB):", sum / n
print "Approx integral (MB·s):", sum * 0.1
}' memory.csvResult: Non Release mode (cago build)one-sided t test Results: Release mode (cargo build --release)one-sided t test Results: non-release Last 5 secondsboth results above didn't show significant memory change. I wondered if using the last 50 data (data from the last 5 seconds) help. Because the yew crates seems to compile at the end: so I took the last 50 samples with tail -n 50 memory.csv | awk '$0 !~ /^#/ { sum += $3 } END {
print "Average memory (MB):", sum / 50
print "Approx integral (MB·s):", sum * 0.1
}'here are the results: one-sided t test: Average memory (MB) : t = 0.742, one-sided p = 0.7620, mean_before = 416.32, mean_after = 420.10 It still doesn't seem to show a reduction in memory usage 🫠 I'm not sure if my methodology is broken, advice welcome! |
Description
Removes most of
.to_string()calls from the impl ofyew-macroby utilising theDisplayimpls of tokens & AST values, analysing them chunk by chunk instead of allocating all of them into a stringChecklist