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: cache nested loops #1677

Merged
merged 1 commit into from
Jan 22, 2025
Merged

fix: cache nested loops #1677

merged 1 commit into from
Jan 22, 2025

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Jan 22, 2025

we're clearly hitting a pathological case in the CSS processing for some folks
my guess is something is repeating the check due to run away mutations or something in a tight loop... or maybe it is just bad at very large strings 🤷

quoting my old colleague Chris Sewart about peformance

you can do it faster
do it less
or not do it

let's cache our results as we go so that we only run the nested check or the regex replace once per input

you can see the test here PostHog/posthog-rrweb#17

we see results like

First execution time: 3.3014169996604323ms
Average execution time for the second execution: 1.8079558699950575ms

that second execution will be O(1)
while first execution is O(n)

so where O(n) is large this should be a much bigger improvement

Copy link

vercel bot commented Jan 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Jan 22, 2025 7:43pm

@pauldambra pauldambra added the bump patch Bump patch version when this PR gets merged label Jan 22, 2025
Copy link

Size Change: +781 B (+0.02%)

Total Size: 3.28 MB

Filename Size Change
dist/all-external-dependencies.js 209 kB +111 B (+0.05%)
dist/array.full.js 370 kB +111 B (+0.03%)
dist/array.full.no-external.js 369 kB +111 B (+0.03%)
dist/module.full.js 370 kB +111 B (+0.03%)
dist/module.full.no-external.js 369 kB +111 B (+0.03%)
dist/recorder-v2.js 116 kB +113 B (+0.1%)
dist/recorder.js 116 kB +113 B (+0.1%)
ℹ️ View Unchanged
Filename Size
dist/array.full.es5.js 266 kB
dist/array.js 182 kB
dist/array.no-external.js 180 kB
dist/customizations.full.js 13.8 kB
dist/dead-clicks-autocapture.js 14.4 kB
dist/exception-autocapture.js 9.48 kB
dist/external-scripts-loader.js 2.64 kB
dist/main.js 182 kB
dist/module.js 182 kB
dist/module.no-external.js 180 kB
dist/surveys-preview.js 67.5 kB
dist/surveys.js 64.2 kB
dist/tracing-headers.js 1.76 kB
dist/web-vitals.js 10.4 kB

compressed-size-action

Copy link

@veryayskiy veryayskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//

@pauldambra pauldambra merged commit d97b413 into main Jan 22, 2025
34 checks passed
@pauldambra pauldambra deleted the fix/cache-nested-loops branch January 22, 2025 20:10
@rafaeelaudibert
Copy link
Member

Memory usage will increase, but that should be fine, lgtm :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants