Skip to content

Conversation

@domenic
Copy link
Member

@domenic domenic commented Jan 9, 2026

The inArray parameter caused cache thrashing: CSSStyleDeclaration.js
called without it (caching strings) while property files called with
inArray: true (expecting arrays). When the cached type didn't match
what a caller needed, it would recompute and overwrite, causing the
next different-type caller to also miss. By always returning arrays,
all callers can share the same cached results.

🤖 Generated with Claude Code


To-do before merging:

  • Human code review (@asamuzaK welcome to help)
  • Double-check benchmark improvements manually
  • Double-check the full jsdom test suite passes

The inArray parameter caused cache thrashing: CSSStyleDeclaration.js
called without it (caching strings) while property files called with
inArray: true (expecting arrays). When the cached type didn't match
what a caller needed, it would recompute and overwrite, causing the
next different-type caller to also miss. By always returning arrays,
all callers can share the same cached results.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
const value = parsers.parsePropertyValue(property, val, {
globalObject,
inArray: true
globalObject
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove all { globalObject }, not just this file.
See #280 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking of including that in this PR but it's simpler to just focus this PR on isArray.

@asamuzaK
Copy link
Contributor

LGTM

@domenic
Copy link
Member Author

domenic commented Jan 10, 2026

Without changes

# style/createElement + setAttribute('style') #
jsdom  x 193 ops/sec ±2.69% (77 runs sampled)

# style/createElement + style properties #
jsdom  x 234 ops/sec ±2.11% (81 runs sampled)

# style/createElement + style.cssText #
jsdom  x 202 ops/sec ±2.49% (80 runs sampled)

# style/innerHTML: divs with inline styles #
jsdom  x 181 ops/sec ±2.50% (79 runs sampled)

# style/innerHTML: simple divs (no styles) #
jsdom  x 5,059 ops/sec ±2.38% (84 runs sampled)

With changes

# style/createElement + setAttribute('style') #
jsdom  x 193 ops/sec ±2.80% (77 runs sampled)

# style/createElement + style properties #
jsdom  x 246 ops/sec ±2.45% (79 runs sampled)

# style/createElement + style.cssText #
jsdom  x 205 ops/sec ±2.93% (78 runs sampled)

# style/innerHTML: divs with inline styles #
jsdom  x 205 ops/sec ±2.32% (79 runs sampled)

# style/innerHTML: simple divs (no styles) #
jsdom  x 4,727 ops/sec ±2.03% (85 runs sampled)

Not a huge improvement but since it makes the code simpler anyway, let's merge.

@domenic domenic merged commit 1257434 into main Jan 10, 2026
3 checks passed
@domenic domenic deleted the perf/remove-inarray branch January 10, 2026 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants