-
Notifications
You must be signed in to change notification settings - Fork 6
refactor(perf): better performance by removing redundant clone operations & lifetimes #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Fix conflicts please |
|
Did you test it to see if it solves the memory leak in dev mode? |
…ove-redundant-clones Signed-off-by: Fahd Ashour <[email protected]>
@hulxv the performance got better. not by much, but it's not ~1GB every refresh. I think deleting cache on my test locally helped too, so I don't have a solid metric i can build upon. Do you have better ideas to measure performance improvemnets for this? You can see the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors code for improved performance by eliminating unnecessary clone operations and simplifying lifetime parameters. The changes focus on reducing memory allocations and improving efficiency in logging and bundling components.
Key changes:
- Removed redundant
clone()calls in the logger by usingremove()instead ofget()operations - Simplified lifetime parameters in
LogFormatstruct and changed ownership model from references to owned values - Removed unnecessary
Clonetrait derivation fromHtmlOutputstruct
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/metassr-watcher/src/utils.rs | Added blank line for code formatting |
| crates/metassr-html/src/builder.rs | Removed Clone derive from HtmlOutput to prevent unnecessary cloning |
| crates/metassr-bundler/src/lib.rs | Changed from to_str().unwrap() to display() for better path handling |
| crates/logger/src/lib.rs | Refactored to eliminate redundant clones and simplified lifetime parameters in LogFormat struct |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| level: &'a Level, | ||
| target: Option<&'a String>, | ||
| message: Option<String>, | ||
| level: &'static Level, |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using &'static Level assumes the Level reference will always have a static lifetime. While this may work for tracing::Level, it's worth verifying that event.metadata().level() actually returns a &'static Level and not a reference with a shorter lifetime.
| level: &'static Level, | |
| level: Level, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
event_metadata().level() actually returns level with reference &Level which returns a const. so it's safe to predict that it has a static life time https://docs.rs/tracing-core/latest/src/tracing_core/metadata.rs.html#505 https://docs.rs/tracing-core/latest/src/tracing_core/metadata.rs.html#66
but Level implements Copy so it's cheap i guess. we can remove it for better readability.
Co-authored-by: Copilot <[email protected]>
No description provided.