-
-
Notifications
You must be signed in to change notification settings - Fork 115
♻️ (blaze/attrs): optimize attribute updates by caching last values #478
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
…o avoid redundant DOM updates and ensure handlers are cleaned up properly
…getUrlProtocol Add standalone HTML files to benchmark and compare performance of ElementAttributesUpdater (_lastValues cache) and getUrlProtocol (with and without protocol cache). These benchmarks help quantify the performance impact of recent optimizations and provide reproducible evidence for future improvements.
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.
Thanks a lot for this improvement! I will thoroughly test this using BlazeUI once we have a alpha release for this, because in BlazeUI I am defining components only around attributes so I can easily tell you if things are breaking.
As I commented, please add more inline docs (Blaze is already missing inline docs a lot, so we need a higher comment to code ratio).
One concern that is left is, whether tests deeply enough covered that caching won't break reactivity and various contexts (with, async, each, helpers etc.) where attributes can play a role.
Expanded and clarified documentation for all attribute handler classes in packages/blaze/attrs.js. Added JSDoc comments, usage examples, and security notes for each handler type, improving maintainability and developer understanding. No functional changes were made.
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 implements performance optimizations for Blaze's attribute system by introducing caching mechanisms to reduce redundant DOM operations. The main focus is optimizing attribute updates by tracking the last known values and only performing DOM updates when values actually change.
- Adds URL protocol caching with LRU eviction to avoid repeated DOM operations for URL validation
- Implements
_lastValues
cache in ElementAttributesUpdater to prevent redundant attribute updates - Updates test application dependencies to Meteor 3.3-beta.1 and enables modern bundle mode
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/blaze/attrs.js |
Core optimization implementation with protocol caching and last-values tracking |
tests/benchmark_getUrlProtocol.html |
Benchmark suite for measuring URL protocol detection performance improvements |
tests/benchmark_ElementAttributesUpdater.html |
Benchmark suite for measuring attribute update performance improvements |
test-app/package.json |
Enables modern bundle mode for the test application |
test-app/.meteor/versions |
Updates Meteor packages to 3.3-beta.1 versions |
test-app/.meteor/release |
Updates Meteor version to 3.3-beta.1 |
test-app/.meteor/packages |
Updates package version references for compatibility |
Comments suppressed due to low confidence (2)
packages/blaze/attrs.js:139
- [nitpick] The shouldUpdate logic is complex and spans multiple lines. Consider extracting this into a helper function like
hasValueChanged(last, value)
to improve readability and testability.
const currentAttrsMap = currentAttrString ? this.parseValue(currentAttrString) : new OrderedDict();
tests/benchmark_getUrlProtocol.html:174
- Using Date.now() for generating unique IDs could cause collisions if multiple elements are created in rapid succession. Consider using a counter or more robust unique ID generation.
console.log(`Average Cached Time: ${avgCachedTime.toFixed(3)} ms`);
This is an experimental PR for some performance improvements.
22e0df1
6d849c8