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

Memory leak on page with many event dispatches #551

Closed
adamraine opened this issue Oct 18, 2024 · 8 comments · Fixed by #554
Closed

Memory leak on page with many event dispatches #551

adamraine opened this issue Oct 18, 2024 · 8 comments · Fixed by #554

Comments

@adamraine
Copy link
Member

adamraine commented Oct 18, 2024 •
edited
Loading

Related DevTools bug: b/369633243

  1. Open DevTools
  2. Go to Settings > Experiments and make sure the "Performance panel: enable live metrics landing page" experiment is disabled
    • This is to ensure the web-vitals.js code shipped with DevTools is not injected
  3. Reload DevTools
  4. Open the DevTools Performance monitor and check "JS event listeners"
  5. Go to the following page and click the button:
<!DOCTYPE HTML>
<script type="module">
  import {onCLS, onINP, onLCP} from 'https://unpkg.com/web-vitals@4?module';

  // Edit: onCLS and onINP don't appear to cause any memory leaks
  // onCLS(console.log);
  // onINP(console.log);
  onLCP(console.log);
</script>
<script>
function spamEvents() {
  for (let i = 0; i < 10_000; i++) {
    // Cannot be GC'd until page reload
    document.dispatchEvent(new MouseEvent('click', {
      view: window,
      clientX: 0,
      clientY: 0
    }));

    // Cannot be GC'd until page reload
    document.dispatchEvent(new MouseEvent('click', {
      view: null,
      clientX: 0,
      clientY: 0
    }));

    // Can be GC'd
    document.dispatchEvent(new MouseEvent('mousedown', {
      view: window,
      clientX: 0,
      clientY: 0
    }));

    // Cannot be GC'd until page reload
    document.dispatchEvent(new KeyboardEvent('keydown', {
      key: 'Enter'
    }));

    // Can be GC'd
    document.dispatchEvent(new KeyboardEvent('keyup', {
      key: 'Enter'
    }));

    // Can be GC'd - (deprecated)
    document.dispatchEvent(new KeyboardEvent('keypress', {
      key: 'Enter'
    }));
  }
}

function round_a_bit(v) {
  // Useful to get garbage off the end of a value that originally came from a 32-bit float.
  return Math.round(v * 10000) / 10000.0;
}

function run() {
  let start = performance.now();
  spamEvents();
  let end = performance.now();
  document.getElementById("output").append(round_a_bit(end - start) + "ms\n");
}
</script>
<input type=button onclick="run()" value=Run>
<pre id="output"></pre>

Expected

(result achieved by removing web-vitals code from the field above)

Screenshot 2024-10-18 at 10 12 59 AM

Actual

Screenshot 2024-10-18 at 10 13 41 AM
@adamraine
Copy link
Member Author

I think I tracked it down to this code in onLCP:

web-vitals/src/onLCP.ts

Lines 100 to 108 in 9b93251

// Stop listening after input. Note: while scrolling is an input that
// stops LCP observation, it's unreliable since it can be programmatically
// generated. See: https://github.com/GoogleChrome/web-vitals/issues/75
['keydown', 'click'].forEach((type) => {
// Wrap in a setTimeout so the callback is run in a separate task
// to avoid extending the keyboard/click handler to reduce INP impact
// https://github.com/GoogleChrome/web-vitals/issues/383
addEventListener(type, () => whenIdle(stopListening), true);
});

Disabling this code will prevent the memory leak.

@tunetheweb
Copy link
Member

Think this is a dupe of #534? It's fixed in v5 which is currently in beta.

@tunetheweb
Copy link
Member

@philipwalton maybe we should consider a final v4 branch with this fix?

@philipwalton
Copy link
Member

@philipwalton maybe we should consider a final v4 branch with this fix?

Did you mean to say v4?

@tunetheweb
Copy link
Member

Yes. For those that don’t want to upgrade to v5 (at least right away) for FID or older browser build reasons.

Then again, not had many complaints about this and Adam’s example is an extreme example.

@philipwalton
Copy link
Member

The base branches for these are very different, so we wouldn't be able to just cherry-pick that commit into v4, but it would be simple enough add {once:true, capture:true} to the addEventListener() call.

@adamraine
Copy link
Member Author

I'm having difficulty rolling 5.0.0-rc.0 into DevTools (possibly an issue with DevTools still investigating) so I think a minor version bump with the changes @philipwalton mentioned would help out a bit here.

@tunetheweb tunetheweb mentioned this issue Oct 22, 2024
Merged
@tunetheweb
Copy link
Member

Yeah I think that makes sense too. Raised #554 for that.

Also super curious to hear about your v5 RC issues—ideally before we finalise that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@philipwalton @adamraine @tunetheweb