-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore: destroy source maps consumers when spec finishes #23708
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
cc @ZachJW34 we talked about source maps in the context of stack traces earlier, this might be relevant/interesting to you. |
|
||
const sourceMapExtractionRegex = /\/\/\s*[@#]\s*sourceMappingURL\s*=\s*(data:[^\s]*)/g | ||
const regexDataUrl = /data:[^;\n]+(?:;charset=[^;\n]+)?;base64,([a-zA-Z0-9+/]+={0,2})/ // matches data urls | ||
|
||
let sourceMapConsumers = {} | ||
let sourceMapConsumers: Record<string, BasicSourceMapConsumer> = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use Map
, I think it makes the intention a bit more clear.
} catch (err) { | ||
// ignore unable to match regex. there's nothing we | ||
// can do about it and we don't want to thrown an exception | ||
if (err.message === 'Maximum call stack size exceeded') return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth checking if this error is the same in FF and WebKit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest checking the error type instead, but it looks like the error type is inconsistent (InternalError in Firefox; RangeError in Chrome and Safari.), but the error message is the same across browsers.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Too_much_recursion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it out - everything works as expected, I did not look into measuring the memory consumption yet (need to do some research on how best to even measure this).
I wanted to know how destroy
worked, it looks like it frees up memory allocated in wasm: https://github.com/mozilla/source-map/blob/master/lib/source-map-consumer.js#L426-L431
Let's see how it goes - either way, this is obviously an improvement, always good to free up memory.
} catch (err) { | ||
// ignore unable to match regex. there's nothing we | ||
// can do about it and we don't want to thrown an exception | ||
if (err.message === 'Maximum call stack size exceeded') return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest checking the error type instead, but it looks like the error type is inconsistent (InternalError in Firefox; RangeError in Chrome and Safari.), but the error message is the same across browsers.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Too_much_recursion
packages/driver/src/cypress.ts
Outdated
@@ -403,15 +403,9 @@ class $Cypress { | |||
break | |||
|
|||
case 'runner:end': | |||
// mocha runner has finished running the tests | |||
$scriptUtils.destroySourceMaps() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we call $sourceMapUtils.destroySourceMapConsumers()
directly, and avoid the indirection of adding this to $scriptUtils?
Co-authored-by: Blue F <blue@cypress.io>
Co-authored-by: Blue F <blue@cypress.io>
Co-authored-by: Bill Glesias <bglesias@gmail.com>
…-io/cypress into destory-source-consumers
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
While profile Cypress to determine the memory impact we have, noticed we aren't explicitly cleaning up our Source Map Consumers. The documentations calls out the need to manually clean this up ourselves to free up wasm data. This may or may not impact our memory footprint, however we felt it'd be worth cleaning up to see if it helps.
https://www.npmjs.com/package/source-map#sourcemapconsumerprototypedestroy
Apart of #23391
Apart of #21135
cypress-documentation
?type definitions
?