Skip to content

perf(utils): rewrite defaults using spread for stable object shape#1514

Draft
spuppo-mux wants to merge 1 commit intovideojs:mainfrom
spuppo-mux:perf/defaults-spread
Draft

perf(utils): rewrite defaults using spread for stable object shape#1514
spuppo-mux wants to merge 1 commit intovideojs:mainfrom
spuppo-mux:perf/defaults-spread

Conversation

@spuppo-mux
Copy link
Copy Markdown
Collaborator

@spuppo-mux spuppo-mux commented May 5, 2026

Refs #1335

Summary

defaults() is called once per setProps() call, which runs on every Lit update cycle for every component. During multi-player init it accumulates into hundreds of calls. The previous imperative for-in loop with dynamic property assignment (result[key as keyof T] = ...) produced result objects with unstable hidden classes in V8, slowing both the function itself and every downstream read of the resulting #props object. Switching to a spread-based functional pipeline preserves the documented contract (undefined values still filtered) while landing on V8's optimized spread fast path.

Changes

  • defaults() rewritten as Object.fromEntries(...filter(...)) + spread merge
  • Behavior unchanged: undefined values still skipped, defaults still fill in
  • Existing 11-test suite passes without modification — the contract is identical, only the implementation strategy changed
Why this is faster

Two V8 mechanisms compound:

  1. Hidden class stability — V8 compiles obj.label to a direct memory offset when the object's shape is predictable. Dynamic property assignment with a computed key (result[key] = value) forces shape transitions and pushes property reads into a slower polymorphic path. Object spread has a heavily optimized fast clone that produces a stable shape.
  2. for-in prototype-chain checkfor (const key in object) traverses the prototype chain by default. V8 has a fast path for plain objects but still verifies enumerability on every call. Object.entries only walks own enumerable keys.

The result objects from defaults() are read repeatedly afterwards (this.#props.label, this.#props.disabled, …), so the hidden-class effect compounds across every property access during init.

Impact

Measured on a 6 player multi-player sandbox html:

Before the changes:
Performance-tab-original
After the changes - different scale, same task goes from ~110ms to ~30ms:
image

  • LCP halves further
  • One Long Task during init eliminated (no >50ms main-thread blocking task remains)

A Long Task disappearing means the engine got a yield point during init that previously didn't exist — paint can land at that yield, which is consistent with the additional LCP improvement.

Testing

pnpm -F @videojs/utils test src/object/tests/defaults.test.ts — existing tests pass without modification, including the cases that exercise the undefined-filtering contract.


Note

Low Risk
Low risk refactor in a small utility that should preserve behavior; main risk is subtle differences in key enumeration/typing due to switching from for...in assignment to Object.entries + spread.

Overview
Refactors defaults() to build a defined object via Object.entries(...).filter(!isUndefined) and then return { ...defaultValues, ...defined } instead of imperatively copying and conditionally assigning properties.

The intent is to keep the same contract (ignore undefined inputs; preserve defined values) while producing a more stable object shape for performance.

Reviewed by Cursor Bugbot for commit 775941d. Bugbot is set up for automated code reviews on this repo. Configure here.

Replaces the imperative for-in loop with a functional pipeline that
filters undefined values and merges via object spread. The behavioral
contract is unchanged (existing tests pass) but the spread-based
construction produces objects with stable hidden classes in V8,
keeping downstream property reads on the fast path. Combined with
removing the for-in's prototype-chain check on every call, this
measurably reduces the cost of defaults() in hot init paths where
setProps is called once per Lit update cycle for every component.

In the multi-player startup benchmark this halves LCP again on top
of the diffing change in videojs#1335 and removes one Long Task during
init.

Refs: videojs#1335
@netlify
Copy link
Copy Markdown

netlify Bot commented May 5, 2026

👷 Deploy request for vjs10-site pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 775941d

@vercel
Copy link
Copy Markdown

vercel Bot commented May 5, 2026

@spuppo-mux is attempting to deploy a commit to the Mux Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 775941d. Configure here.


return result;
const defined = Object.fromEntries(Object.entries(object).filter(([, value]) => !isUndefined(value)));
return { ...defaultValues, ...defined } as T;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Object.entries skips inherited reactive element properties

High Severity

Object.entries() only returns own enumerable properties, but ReactiveElement installs reactive properties as prototype accessors. The previous for...in loop correctly traversed the prototype chain, but the new code silently skips these properties. As a result, defaults() ignores user-set properties, always returning default values. This affects custom elements using setProps(this), making their property customization non-functional.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 775941d. Configure here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed this is why this was much faster, it was skipping many attributes

@spuppo-mux spuppo-mux marked this pull request as draft May 6, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant