chore: destroy source maps consumers when spec finishes #23708
Conversation
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run 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.
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.
Might be worth checking if this error is the same in FF and WebKit
There was a problem hiding this comment.
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.
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.
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.
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?