Cache element styles for getComputedStyle

[jsnajdr]

Fixes #3234. Caches the computed styles for each element, avoiding expensive re-calculations. Also, modifies the implementation of getResolvedValue to not match all the style rules for itself, but reuse the cached results.

Each document has its own cache, and the cache is completely invalidated on every DOM change. I looked up all queueMutationRecord calls and added an invalidateStyleCache call to every place that also triggers a mutation observer.

What are the actual performance improvements? I tested it on one of our React Testing Library unit test suites in the Gutenberg project. Before the patch, the suite takes 11.0 s to run:

 PASS  packages/components/src/border-box-control/test/index.js (10.066 s)
  BorderBoxControl
    Linked view rendering
      ✓ should render correctly when no value provided (311 ms)
      ✓ should hide label (26 ms)
      ✓ should show correct width value when flat border value provided (70 ms)
      ✓ should show correct width value when consistent split borders provided (55 ms)
      ✓ should render placeholder and omit unit select when border values are mixed (184 ms)
      ✓ should render shared border width and unit select when switching to linked view (202 ms)
      ✓ should omit style options when requested (491 ms)
    Split view rendering
      ✓ should render split view by default when mixed values provided (376 ms)
      ✓ should render correct width values in appropriate inputs (191 ms)
      ✓ should render split view correctly when starting with flat border (250 ms)
      ✓ should omit style options when requested (2902 ms)
    onChange handling
      Linked value change handling
        ✓ should set undefined when new border is empty (65 ms)
        ✓ should update with complete flat border (100 ms)
        ✓ should maintain mixed values if not explicitly set via linked control (178 ms)
        ✓ should update with consistent split borders (91 ms)
        ✓ should set undefined borders when change results in empty borders (63 ms)
        ✓ should set flat border when change results in consistent split borders (182 ms)
      Split value change handling
        ✓ should set split borders when the updated borders are mixed (222 ms)
        ✓ should set flat border when updated borders are consistent (227 ms)

Test Suites: 1 passed, 1 total
Tests:       19 passed, 19 total
Snapshots:   0 total
Time:        10.989 s

After the patch, it takes only 7.6 s to run, a 30% improvement:

 PASS  packages/components/src/border-box-control/test/index.js (6.845 s)
  BorderBoxControl
    Linked view rendering
      ✓ should render correctly when no value provided (257 ms)
      ✓ should hide label (44 ms)
      ✓ should show correct width value when flat border value provided (60 ms)
      ✓ should show correct width value when consistent split borders provided (55 ms)
      ✓ should render placeholder and omit unit select when border values are mixed (184 ms)
      ✓ should render shared border width and unit select when switching to linked view (149 ms)
      ✓ should omit style options when requested (180 ms)
    Split view rendering
      ✓ should render split view by default when mixed values provided (151 ms)
      ✓ should render correct width values in appropriate inputs (93 ms)
      ✓ should render split view correctly when starting with flat border (138 ms)
      ✓ should omit style options when requested (727 ms)
    onChange handling
      Linked value change handling
        ✓ should set undefined when new border is empty (46 ms)
        ✓ should update with complete flat border (68 ms)
        ✓ should maintain mixed values if not explicitly set via linked control (147 ms)
        ✓ should update with consistent split borders (55 ms)
        ✓ should set undefined borders when change results in empty borders (39 ms)
        ✓ should set flat border when change results in consistent split borders (125 ms)
      Split value change handling
        ✓ should set split borders when the updated borders are mixed (108 ms)
        ✓ should set flat border when updated borders are consistent (105 ms)

Test Suites: 1 passed, 1 total
Tests:       19 passed, 19 total
Snapshots:   0 total
Time:        7.625 s

There's one test that's particularly heavy on getComputedStyle calls:

Split view rendering > should omit style options when requested

This one improves from 2.90s to 0.73s, an 75% improvement, or 4x faster.

The pointer-events patch in #3478 adds more perf improvements on top of that. The test run looks like:

 PASS  packages/components/src/border-box-control/test/index.js
  BorderBoxControl
    Linked view rendering
      ✓ should render correctly when no value provided (201 ms)
      ✓ should hide label (25 ms)
      ✓ should show correct width value when flat border value provided (43 ms)
      ✓ should show correct width value when consistent split borders provided (43 ms)
      ✓ should render placeholder and omit unit select when border values are mixed (159 ms)
      ✓ should render shared border width and unit select when switching to linked view (127 ms)
      ✓ should omit style options when requested (154 ms)
    Split view rendering
      ✓ should render split view by default when mixed values provided (120 ms)
      ✓ should render correct width values in appropriate inputs (78 ms)
      ✓ should render split view correctly when starting with flat border (125 ms)
      ✓ should omit style options when requested (596 ms)
    onChange handling
      Linked value change handling
        ✓ should set undefined when new border is empty (39 ms)
        ✓ should update with complete flat border (54 ms)
        ✓ should maintain mixed values if not explicitly set via linked control (118 ms)
        ✓ should update with consistent split borders (50 ms)
        ✓ should set undefined borders when change results in empty borders (35 ms)
        ✓ should set flat border when change results in consistent split borders (122 ms)
      Split value change handling
        ✓ should set split borders when the updated borders are mixed (97 ms)
        ✓ should set flat border when updated borders are consistent (94 ms)

Test Suites: 1 passed, 1 total
Tests:       19 passed, 19 total
Snapshots:   0 total
Time:        5.223 s, estimated 11 s

The 5.2 s time, compared to 7.6 s without the pointer-events patch, means another 30% improvement. Due to faster style lookup during userEvent.click.

[domenic]

Really great work. Some suggestions for how to clean it up.

Bonus points (but not required) if you add extra to-upstream WPT coverage as follows:

• Remove all current calls to invalidateStyleCache
• For each such current call, add a test that fails. Then, add the call back, and ensure the new test passes.
• Finally, consolidate all the calls as I suggested here into _modified, instead of having them be spread out like they are here.

All such tests could be in the same file, for simplicity.

I think at least one such test would help a lot, by providing a template in case we notice future regressions or over-caching instances.

[domenic]

I think all, or maybe almost-all, invalidStyleCache calls can be moved into

jsdom/lib/jsdom/living/nodes/Node-impl.js

Line 215 in 22f7c3c

_modified() {

[domenic]

_ownerDocument should never be null. (Here and elsewhere.)

[domenic]

Instead of a weak map, why not put this on the document itself? Combined with the non-nullness of _ownerDocument, that could greatly reduce the lines of code needed to access the style cache. Just have the weak map always exist.

[domenic]

I'm not sure this is necessary? When could changing text data cause a style update?

[jsnajdr]

Thanks @domenic for your feedback! I need to finish some other tasks before I get back to this, but I plan to implement the TDD-style tests as you suggested, and will try if moving the invalidation to the _modified method is viable.

_ownerDocument should never be null.

I didn't know that, it's true that it will help to simplify the implementation quite a bit.

I'm not sure this is necessary? When could changing text data cause a style update?

There's the :empty pseudo-class, which might be not be applied when text data change? Or is that fully covered by addition and removal of a text node? The unit tests will tell 🙂

[jsnajdr]

Hi @domenic 👋 I updated the PR according to your feedback. The style cache is now directly on the document, I removed the weak map from documents to caches.

Also, I implemented a test suite, gradually adding tests for things like attribute or node changes, and adding invalidateStyleCache where needed. Turns out that almost all of them could really be eventually consolidated into _modified().

I'm not sure this is necessary? When could changing text data cause a style update?

I found one case where text data change can trigger style change. Try this in a browser:

const el = document.createElement("div");
el.textContent = "foo";
document.body.append(el);

Now try el.matches("div:empty") and it will return false, as expected, because the element contains "foo". Now do this:

el.childNodes[0].data = "";

This makes the node empty, although it still has one child text node, and el.matches("div:empty") returns true.

However, JSDOM currently implements this incorrectly, because nwsapi's :empty implementation says "not empty" when it sees a child text node.

Merely setting el.textContent = "" would work, because it involves removing a Node, and the invalidation in _modified() is triggered.

To make this forward-compatible, I also invalidate styles in _childTextContentChangeSteps(), called after a child text node is updated. And the test for :empty uses the el.textContent method, and the test code that requires the text-data-change invalidation to pass is commented out.

---

While working on the PR, I discovered a bug: when a <link rel="stylesheet"> element is removed from the document, its stylesheet is not removed from document.styleSheets. Its styles continue to be active. I fixed this in a separate commit, "remove stylesheet from document when the link element is removed", which could be its own PR, it's self-contained.

When a stylesheet is removed or created, that's another place where we need to invalidate style cache.

[jsnajdr]

Here's how Chromium implements the :empty check: https://github.com/chromium/chromium/blob/main/third_party/blink/renderer/core/css/selector_checker.cc#L1262-L1290

It really looks at the character data of child text nodes and checks if they are empty. There's also preparatory code to start accepting also whitespace-only text as :empty, as the standard has changed in that direction.

[domenic]

Really great work; thank you so much!

[jsnajdr]

@domenic Do you think this could be backported to the 20.x.x line, so that users of Jest can get the performance improvements right away? Otherwise we'll need to wait for jest-environment-jsdom update, and its jsdom dependency is usually bumped only on Jest major releases...

[domenic]

No, sorry, we only have bandwidth to maintain a single release of jsdom. I suggest working with the maintainers of that other package.

[jsnajdr]

For record, this is a Jest PR to upgrade jest-environment-jsdom to JSDOM 21: jestjs/jest#13825. It's not a simple version bump, but also requires Jest changes in how it works with the now read-only fields like window.document.