Replay hinders performance on Angular apps

[billyvg]

Related to https://github.com/getsentry/sentry-javascript/issues/6946#issuecomment-1875842171 - Replay can still cause performance regressions for Angular apps when calling the Angular-patched versions of certain native functions that results in trigger a massive amount of Angular's change detectors.

We only changed to use the non-patched version of requestAnimationFrame, but it's possible this happens with other timer-related functions as well. From looking at [the minified] Angular source:

I don't know Angular at all, but we may want to try to use the unpatched versions of these functions:

• [ ] setTimeout
• [ ] clearTimeout
• [ ] setInterval
• [ ] clearInterval
• [ ] setImmediate
• [ ] clearImmediate

Ideally, this would happen only in the angular package as to not bloat the rest of the packages.

Closes https://github.com/getsentry/team-replay/issues/424 (Internal ticket)

[mydea]

Hmm, that kind of sucks 😅

So, if this is the same issue as with requestAnimationFrame, it is only relevant to do this in rrweb functions that are related to capturing etc, I guess. So we don't necessarily have to wrap all invocations, only the ones in "hot paths" I guess 🤔

setImmediate / clearImmediate are not used in rrweb, so we can disregard these for now.

setInterval / clearInterval is also not used in any package we use, so probably also fine to ignore.

setTimeout / clearTimeout are used quite a lot 😬 So maybe we can just use these... I can give it a try, maybe we can make the implementation generic so the bundle size impact is negligible 🤔

[billyvg]

Re-opening this as it was auto-closed from a PR that only partially addresses this issue. We have some setTimeout usage in our Replay SDK that needs to also call the unpatched version of setTimeout as to not conflict with Angular's change detection.

[natebundy]

@billyvg @mydea Is it possible for you all to run this code outside of the Angular zone to avoid this issue entirely? "Third-party libraries commonly trigger unnecessary change detection cycles because they weren't authored with Zone.js in mind. Avoid these extra cycles by calling library APIs outside the Angular zone"

See the Angular docs: https://angular.io/guide/change-detection-zone-pollution https://angular.io/api/core/NgZone#runoutsideangular

[natebundy]

Thinking about this more, since Sentry is initializing outside Angular, you could possibly look for the global Zone object existing and call things in Zone.root.run(() => {...}) to avoid change detection cycles. https://github.com/angular/angular/blob/7287fefe7a750a1ae584d9bb9e0378aa047b5d95/packages/zone.js/lib/zone-impl.ts#L129C1-L133C40

However, since I'm assuming Sentry is generally initializing before Zone.js, that may not be any better than keeping around copies of non-monkey patched functions, unless you have a single or few entry points that you could call within Zone.root.run(() => {...}).

Another (not great) option would be to wrap Sentry in its own zone from Zone.js during initialization. As long as the zone isn't "angular" (e.g. a new "sentry" zone) it won't mess with change detection. It seems like Angular needs a better way to opt-out of zones for JavaScript libraries.

(The app I'm working on has a performance degradation in a virtual scrolling table with Sentry enabled due to all of the extra change detection cycles, so I'm invested!)

[billyvg]

@natebundy we just released version 8.3.0 that includes changes to use an unpatched setTimeout, would you be able to see if it helps with your virtual table?

Unfortunately I don't think our SDKs are flexible enough to run certain code paths outside of Angular's zones.

[natebundy]

Thanks, I'll give it a try!

I just found out Angular is experimenting with zoneless change detection, so hopefully at some point in the very far future this will stop being an issue.