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.

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

fix: skip custom split #21812

Merged
merged 1 commit into from
Apr 24, 2024
Merged

fix: skip custom split #21812

merged 1 commit into from
Apr 24, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Apr 24, 2024

Problem

Looking at the performance inspector we can see CPU spiking on the main thread and a ton of time being taken up in the customSplit method
Screenshot 2024-04-24 at 15 05 47

This was introduced in rrweb-io/rrweb#1401 which lines up with reports we've been getting of playback freezing in the last couple of days (we upgraded to alpha-13 yesterday #21734)

Changes

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Should see customers recordings playing back correctly

@daibhin daibhin merged commit ab3a64d into master Apr 24, 2024
97 checks passed
@daibhin daibhin deleted the dn-fix/skip-custom-split branch April 24, 2024 14:30
@daibhin daibhin requested a review from a team April 24, 2024 14:30
@daibhin
Copy link
Contributor Author

daibhin commented Apr 24, 2024

@pauldambra @marandaneto I was totally over-eager here. I saw all green and accidentally yolo merged. It feels like a relatively safe change given things are pretty broken right now and I'm fairly confident this will fix things.

@marandaneto
Copy link
Member

@pauldambra @marandaneto I was totally over-eager here. I saw all green and accidentally yolo merged. It feels like a relatively safe change given things are pretty broken right now and I'm fairly confident this will fix things.

All good, you have 6 YOLOs left, kidding, hopefully this fix everything o/

@pauldambra
Copy link
Member

would be interesting to grab some of the real-world input that was causing this and see if we can write tests that demonstrate the problem so we can drive a fix

(all the regexes in that chain of code scare me 🤣)

@daibhin
Copy link
Contributor Author

daibhin commented Apr 24, 2024

Sadly #21816 didn't fix the issue so I've gone ahead and removed the entire suspect commit.

@pauldambra think you're right. I'm working on a move to https://github.com/csstree/csstree which has proper CSS ast support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants