Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Bug]: splitCssText causes degraded performance when recording #1603

Closed
1 task done
guntherjh opened this issue Dec 6, 2024 · 4 comments
Closed
1 task done

[Bug]: splitCssText causes degraded performance when recording #1603

guntherjh opened this issue Dec 6, 2024 · 4 comments
Assignees
@eoghanmurray
Labels
bug Something isn't working

Comments

@guntherjh
Copy link

guntherjh commented Dec 6, 2024 •
edited
Loading

Preflight Checklist

  • I have searched the issue tracker for a bug report that matches the one I want to file, without success.

What package is this bug report for?

rrweb

Version

v2.0.0-alpha.18

Expected Behavior

rrweb should not have a significant impact on an application/site when recording data.

Actual Behavior

Whenever markCssSplits is called, we have observed significantly degraded performance caused by splitCssText maxing out the JS heap.

Steps to Reproduce

Reproduction steps with browser extensions:
In the wild, we were seeing this issue with customers who were using chrome extensions that are injecting CSS in to the page. We've seen the issue with our customers using the Grammarly browser extension and Sider AI browser extension. To reproduce the issue this way:

  1. Install one these two extensions.
  2. Open a page that has rrweb recordind data.
  3. In the case of Grammarly, you just need to click in an input in a form and that should trigger the extension to start injecting code that will trigger the issue. For Sider AI, you need to open the extension and a side bar will appear that triggers the issue.

Simple reproduction steps (not using a browser extension):

  1. Serve the following HTML file using a simple http server. For example. I used http-server.
<html>
<head>
    // test.css can really contain anything, it's super important for this test
    <link href="test.css"rel="stylesheet">
    // benchmark.css is the css file used for benchmarking in /packages/rrweb-snaphsot/test/css
    // choosen b/c it's pretty large
    <link href="benchmark.css"rel="stylesheet">
    <title>Home</title>
    <textarea></textarea>
    <script type="module">
        // This is the record.js file from /packages/record/dist created after running yarn && yarn build:all in the root of the repo
        // commented out for now
        // import { record } from './record.js';
        
        const stylesheet = document.getElementsByTagName('link')[1].sheet; 

        let cssString = '';
        // Check if the stylesheet is loaded
        if (stylesheet.rules) {
            // Get all the CSS rules as a string
            for (let i = 0; i < stylesheet.cssRules.length; i++) {
            cssString += stylesheet.cssRules[i].cssText;
            }
        }

        var recorder = record({
            emit(event) {
                console.log(event);
            },
        });
        
        const newStyleEl = document.createElement('style');
        document.body.appendChild(newStyleEl);
        // adds Text Nodes to the <style> element so that markCssSplits gets executed.
        newStyleEl.appendChild(document.createTextNode(cssString));
        newStyleEl.appendChild(document.createTextNode(cssString));
    </script>
</html>
  1. Open the URL were the HTML file is being served. For my example that was http://127.0.0.1:8080.
  2. Using Google Chrome, start taking a performance profile.
  3. Uncomment this line // import { record } from './record.js'; and refresh the page.
  4. Stop recording the performance profile. Note this seems to be enough to lock up the browser so you may need to refresh to get the performance profile to finish loading.
  5. You should see something akin to the following in the performance profile:
    Screenshot 2024-12-06 at 3 12 17 PM

Testcase Gist URL

No response

Additional Information

Just for reference, here is the commit that added splitCssTest

This issue seems to be related, but looks to be more of a problem on the playback side (unless I am misunderstanding 😅 ).

Benchmark
IDK how helpful this would be, but I created a simple benchmark for splitCssText in /packages/rrweb-snapshot/test/stringify-stylesheet.bench.ts

describe('splitCssText', () => {
  const style: HTMLStyleElement = document.createElement('style');
  const style2: HTMLStyleElement = document.createElement('style');

  const cssText = fs.readFileSync(
    path.resolve(__dirname, './css/benchmark.css'),
    'utf8',
  );
  style.textContent = cssText;
  style2.textContent = cssText;
  style2.appendChild(document.createTextNode(cssText));
  style2.appendChild(document.createTextNode(cssText));
  bench(
    'splitCssText',
    () => {
      splitCssText(cssText, style);
    },
    { time: 1000},
  );
  bench(
    'splitCssText triggers nested for loops',
    () => {
      splitCssText(cssText, style2);
    },
    { time: 1000},
  );
});

Here are the results I got:
Screenshot 2024-12-06 at 3 25 05 PM

Please let me know if there is any additional details I should provide. TY!
@guntherjh guntherjh added the bug Something isn't working label Dec 6, 2024
@guntherjh
Copy link
Author

Update on the reproduction steps above. I have also observed that when a style element contains a large amount of CSS, the resulting CSS gets split in to multiple child text nodes of the style element. This looks to happen at around 60-70kB of CSS in Chrome. There doesn't seem to be a defined limit (at least that I can find). In the case of the Grammarly browser extension, the injected web component contains a style element with 104kB of CSS that gets split across two child text nodes (the first containing 65.6kB and the second containing the remaining 38.3kB). I also observed a web component on a customer's site that had a style element containing what appeared to be all of the font awesome CSS library. This got split across multiple child text nodes in a similar manner.

@eoghanmurray eoghanmurray self-assigned this Dec 12, 2024
@eoghanmurray eoghanmurray mentioned this issue Dec 18, 2024
Merged
@eoghanmurray
Copy link
Contributor

I've a PR with a fix in #1615

While the benchmark demonstrates a huge difference in performance between the two cases, there's no actual improvement in the benchmark after the PR as it wasn't demonstrating the real pathological performance; the difference in the two bench timings was just reflecting the necessary call to `normalizeCssString` and accessing of childNodes content.

I've instead made a pathological case out of your test html file, and added an iter_limit so that we 'early out' if we are iterating too much.

Thanks so much for the bug report!

@guntherjh
Copy link
Author

@eoghanmurray Thanks for taking a look! TBH I didn't have a ton of time to analyze the benchmark I put up so what you described makes perfect sense.

@github-actions github-actions bot mentioned this issue Jan 10, 2025
Open
@hipporello
Copy link

Hi,

I was testing this after your fix and it seems that splitCssText hits the performance severely still. It freeze the screen for 10 secs. May I ask why we need this method?

This was referenced Jan 16, 2025
Open
Open
Open
@kevinansfield kevinansfield mentioned this issue Jan 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eoghanmurray eoghanmurray

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@eoghanmurray @guntherjh @hipporello