♻️ (blaze/attrs): optimize attribute updates by caching last values #478

wreiske · 2025-05-22T04:28:54Z

This is an experimental PR for some performance improvements.

22e0df1

6d849c8

…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.

jankapunkt

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.

packages/blaze/attrs.js

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.

wreiske · 2025-07-24T22:54:03Z

Copilot

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`);

packages/blaze/attrs.js

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-12T09:30:41Z

tests/benchmark_ElementAttributesUpdater.html

+      if (handler) { // Proceed only if a handler exists or was created
+        const lastCachedValue = lastValues[k]; // Read from cache
+        // The crucial check using _lastValues
+        const shouldUpdate = (lastCachedValue !== newValueForAttr && String(lastCachedValue) !== String(newValueForAttr)); // Changed || to &&


The logic in the benchmark doesn't match the actual implementation in attrs.js. The benchmark uses a simple comparison (lastCachedValue !== newValueForAttr && String(lastCachedValue) !== String(newValueForAttr)) on line 138, but the actual implementation in attrs.js uses a more complex condition with additional null handling logic on lines 736-738. This inconsistency means the benchmark isn't accurately testing the actual code behavior.

Suggested change

const shouldUpdate = (lastCachedValue !== newValueForAttr && String(lastCachedValue) !== String(newValueForAttr)); // Changed || to &&

// Match the null-handling logic from attrs.js

const shouldUpdate = (

lastCachedValue !== newValueForAttr &&

!(lastCachedValue == null && newValueForAttr == null) &&

String(lastCachedValue) !== String(newValueForAttr)

);

Copilot · 2025-12-12T09:30:41Z

packages/blaze/attrs.js

+    // Performance optimization: only update if value actually changed
+    // This handles edge cases with null/undefined values and type coercion
+    const last = lastValues[k];
+    const shouldUpdate = last !== value && 
+                        !((last == null && value == null) || 
+                          (typeof last === typeof value && String(last) === String(value)));


The shouldUpdate logic has a flaw when handling new attributes. When an attribute is first created (line 724), lastValues[k] is undefined. The condition on lines 736-738 evaluates:

last !== value → undefined !== value (true for non-undefined values)

(last == null && value == null) → true only if value is also null/undefined

String(last) === String(value) → "undefined" === String(value)

This means a newly created attribute with a non-null value will correctly trigger an update. However, if value is the literal string "undefined", the String comparison will be true, causing the update to be skipped incorrectly. Additionally, the logic doesn't initialize lastValues[k] when the handler is created, which means subsequent calls will continue treating it as undefined until the first update occurs.

Copilot · 2025-12-12T09:30:42Z

packages/blaze/attrs.js

+    const shouldUpdate = last !== value && 
+                        !((last == null && value == null) || 


The shouldUpdate condition performs a String() conversion on every check (line 738), which has a performance cost. For attributes that update frequently, this string conversion happens on every render cycle even when values haven't changed. This partially defeats the purpose of the optimization. Consider checking strict equality first, and only falling back to string comparison if that fails, or moving the string comparison to a separate branch to avoid unnecessary conversions.

Suggested change

const shouldUpdate = last !== value &&

!((last == null && value == null) ||

const shouldUpdate = !(last === value ||

(last == null && value == null) ||

Copilot · 2025-12-12T09:30:42Z

packages/blaze/attrs.js

-    return (anchorForNormalization.protocol || "").toLowerCase();
+    const protocol = (anchorForNormalization.protocol || "").toLowerCase();
+
+    // Implement simple LRU cache eviction to prevent memory leaks


The comment on line 526 says "Implement simple LRU cache eviction" but the code actually implements FIFO (First In, First Out) eviction, not LRU (Least Recently Used). LRU would require updating an item's position when it's accessed via get(), which this implementation doesn't do. The comment should be updated to reflect the actual behavior.

Suggested change

// Implement simple LRU cache eviction to prevent memory leaks

// Implement simple FIFO cache eviction to prevent memory leaks

Copilot · 2025-12-12T09:30:42Z

tests/benchmark_ElementAttributesUpdater.html

+  function runBenchmarkIteration(UpdaterConstructor, description, statusElement, suppressConsoleLog = false) {
+    const testElement = document.createElement('div');
+    // Assign unique ID for potential debugging, though not strictly needed for logic
+    testElement.id = description.replace(/\s+/g, '') + "_" + Date.now(); // Corrected regex replacement string 


The comment states "Corrected regex replacement string" but there's no indication of what was corrected or why this comment is necessary. The code description.replace(/\s+/g, '') is standard JavaScript for removing all whitespace, and there's no apparent correction. This comment should either be removed or clarified to explain what issue was corrected.

Suggested change

testElement.id = description.replace(/\s+/g, '') + "_" + Date.now(); // Corrected regex replacement string

testElement.id = description.replace(/\s+/g, '') + "_" + Date.now();

The comment states "Corrected regex replacement string" but there's no indication of what was corrected or why this comment is necessary

Copilot · 2025-12-12T09:30:43Z

packages/blaze/attrs.js

 const getUrlProtocol = function (url) {
+  // Check cache first for performance
+  if (_protocolCache.has(url)) {
+    return _protocolCache.get(url);
+  }
+
  if (Meteor.isClient) {
+    // Use browser's URL parsing via anchor element
    anchorForNormalization.href = url;
-    return (anchorForNormalization.protocol || "").toLowerCase();
+    const protocol = (anchorForNormalization.protocol || "").toLowerCase();
+
+    // Implement simple LRU cache eviction to prevent memory leaks
+    if (_protocolCache.size >= MAX_CACHE_SIZE) {
+      // Remove oldest entry (first key in insertion order)
+      const firstKey = _protocolCache.keys().next().value;
+      _protocolCache.delete(firstKey);
+    }
+
+    // Cache the result for future use
+    _protocolCache.set(url, protocol);
+    return protocol;
  } else {
    throw new Error('getUrlProtocol not implemented on the server');
  }
 };


The getUrlProtocol function doesn't validate the input parameter url. If url is not a string (e.g., null, undefined, number, object), setting it as anchorForNormalization.href could cause unexpected behavior or errors. While JavaScript will coerce these values to strings, this could lead to incorrect cache entries and protocol detection. For example, null becomes the string "null", and undefined becomes "undefined", which would be parsed as relative URLs and could produce unexpected protocols.

Copilot · 2025-12-12T09:30:43Z

packages/blaze/attrs.js

+/**
+ * Protocol cache for performance optimization
+ * 
+ * Since URL protocol detection requires DOM manipulation, we cache results
+ * to avoid repeated operations on the same URLs. Uses LRU-style eviction
+ * to prevent memory leaks in long-running applications.


The documentation states "Uses LRU-style eviction to prevent memory leaks" but the implementation is not actually LRU (Least Recently Used). The code removes the oldest entry by insertion order (FIFO - First In, First Out), not the least recently accessed entry. In true LRU, accessing an existing cached value would move it to the end of the queue, but this implementation doesn't do that - it only checks has() and returns the value without updating its position. The documentation should be corrected to say "Uses FIFO eviction" or the implementation should be updated to properly implement LRU.

Copilot · 2025-12-12T09:30:43Z

packages/blaze/attrs.js

+const _protocolCache = new Map(); 
+const MAX_CACHE_SIZE = 1000;
+
+/**


The protocol cache is shared globally across all ElementAttributesUpdater instances and persists for the lifetime of the application. If a URL's protocol changes during the application lifecycle (which is theoretically possible if the page's base URL changes or if relative URLs are used), the cached protocol would be incorrect. Additionally, there's no mechanism to clear or reset the cache, which could be problematic for testing or specific use cases. Consider adding a cache clearing function or documenting this limitation.

Suggested change

/**

/**

* Clear the protocol cache.

*

* This is useful for testing or in cases where the base URL or URL resolution context changes

* during the application lifecycle. Use with caution in production.

*/

Blaze._clearProtocolCache = function () {

_protocolCache.clear();

};

/**

italojs

LGTM, we have a unecessary comment but I'll aprove it

italojs · 2025-12-12T12:55:36Z

tests/benchmark_ElementAttributesUpdater.html

+  function runBenchmarkIteration(UpdaterConstructor, description, statusElement, suppressConsoleLog = false) {
+    const testElement = document.createElement('div');
+    // Assign unique ID for potential debugging, though not strictly needed for logic
+    testElement.id = description.replace(/\s+/g, '') + "_" + Date.now(); // Corrected regex replacement string 


The comment states "Corrected regex replacement string" but there's no indication of what was corrected or why this comment is necessary

italojs

I'm sorry, never mind the last approval. I didn't see the copilot review and need to take a better look.
BTW: I got surprised with that AI Review

italojs · 2025-12-12T13:11:36Z

tests/benchmark_ElementAttributesUpdater.html

+      if (handler) { // Proceed only if a handler exists or was created
+        const lastCachedValue = lastValues[k]; // Read from cache
+        // The crucial check using _lastValues
+        const shouldUpdate = (lastCachedValue !== newValueForAttr && String(lastCachedValue) !== String(newValueForAttr)); // Changed || to &&


Can you remove this comment, please?

jankapunkt · 2025-12-17T07:39:00Z

CI now reports

 While processing files with ecmascript (for target os.linux.x86_64):
   packages/babel-compiler.js:126:18: Plugin?.getMeteorConfig is not a
   function
   at getMeteorConfig (packages/babel-compiler.js:126:18)
   at BabelCompiler.BCp.initializeMeteorAppConfig
   (packages/babel-compiler.js:189:24)
   at BabelCompiler.BCp.processFilesForTarget
   (packages/babel-compiler.js:294:8)
   
   
   While building package templating-runtime:
   error: No plugin known to handle file 'dynamic.html'. If you want this file
   to be a static asset, use addAssets instead of addFiles; eg,
   api.addAssets('dynamic.html', 'client').
   
   While building package local-test:templating-runtime:
   error: No plugin known to handle file 'dynamic_tests.html'. If you want this
   file to be a static asset, use addAssets instead of addFiles; eg,
   api.addAssets('dynamic_tests.html', 'client').

@wreiske does this occur on your local tests as well?

♻️ (blaze/attrs): optimize attribute updates by caching last values t…

22e0df1

…o avoid redundant DOM updates and ensure handlers are cleaned up properly

wreiske mentioned this pull request May 22, 2025

DRAFT: Blazing! #476

Closed

wreiske added 2 commits May 21, 2025 21:53

♻️ (blaze/attrs.js): add simple cache to getUrlProtocol for performance

6d849c8

jankapunkt requested a review from Copilot May 22, 2025 06:45

This comment was marked as outdated.

Sign in to view

jankapunkt requested changes May 22, 2025

View reviewed changes

packages/blaze/attrs.js Outdated Show resolved Hide resolved

packages/blaze/attrs.js Show resolved Hide resolved

packages/blaze/attrs.js Outdated Show resolved Hide resolved

jankapunkt and others added 2 commits May 22, 2025 21:25

Merge branch 'master' into feature/blaze-perf

cabec74

wreiske marked this pull request as ready for review July 24, 2025 22:54

wreiske requested review from Copilot and jankapunkt July 24, 2025 22:54

Copilot AI reviewed Jul 24, 2025

View reviewed changes

packages/blaze/attrs.js Show resolved Hide resolved

packages/blaze/attrs.js Show resolved Hide resolved

wreiske changed the title ~~DRAFT: ♻️ (blaze/attrs): optimize attribute updates by caching last values~~ ♻️ (blaze/attrs): optimize attribute updates by caching last values Jul 24, 2025

jankapunkt requested a review from Copilot December 12, 2025 09:24

Copilot started reviewing on behalf of jankapunkt December 12, 2025 09:24 View session

Copilot AI reviewed Dec 12, 2025

View reviewed changes

italojs approved these changes Dec 12, 2025

View reviewed changes

italojs requested changes Dec 12, 2025

View reviewed changes

Merge branch 'master' into feature/blaze-perf

02b2f95

jankapunkt added this to the 3.1 milestone Dec 17, 2025

		const shouldUpdate = last !== value &&
		!((last == null && value == null) \|\|

	// Implement simple LRU cache eviction to prevent memory leaks
	// Implement simple FIFO cache eviction to prevent memory leaks

	testElement.id = description.replace(/\s+/g, '') + "_" + Date.now(); // Corrected regex replacement string
	testElement.id = description.replace(/\s+/g, '') + "_" + Date.now();

-/**
+/**
+ * Clear the protocol cache.
+ *
+ * This is useful for testing or in cases where the base URL or URL resolution context changes
+ * during the application lifecycle. Use with caution in production.
+ */
+Blaze._clearProtocolCache = function () {
+  _protocolCache.clear();
+};
+/**

Uh oh!

♻️ (blaze/attrs): optimize attribute updates by caching last values #478

Are you sure you want to change the base?

♻️ (blaze/attrs): optimize attribute updates by caching last values #478

Uh oh!

Conversation

wreiske commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

jankapunkt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wreiske commented Jul 24, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

italojs Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

italojs left a comment

Choose a reason for hiding this comment

Uh oh!

italojs Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

italojs left a comment

Choose a reason for hiding this comment

Uh oh!

italojs Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

jankapunkt commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wreiske commented May 22, 2025 •

edited

Loading