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

Remove pretty printing from register cache #14300

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Remove pretty printing from register cache #14300

merged 1 commit into from
Feb 24, 2022

Conversation

jlennox
Copy link
Contributor

@jlennox jlennox commented Feb 23, 2022

We're hitting some sort of memory issue with register's caching. A workaround is to remove the pretty printing.

When I deleted node_modules/.cache/@babel/register/.babel.7.13.10.development.json

  • then run with JSON.stringify(data, null, " "); the re-created file is 152,577kb.
  • then run with JSON.stringify(data); the re-created file is 109,352kb.

Since there's no reason to store a cache file pretty printed, we can reduce the memory pressure by ~30% by not doing that.

Q                       A
Fixed Issues?
Patch: Bug Fix? Reduces occurrences of /issues/5667
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? No
Documentation PR Link
Any Dependency Changes? No
License MIT

The crash report I'm seeing is:

1% setup initializeStacktrace:
   ptr1=00000025696C0541
    ptr2=0000000000000000
    ptr3=0000000000000000
    ptr4=0000000000000000
    failure_message_object=0000000FA9B86EE0

==== JS stack trace =========================================

    0: ExitFrame [pc: 00007FF741B2CF5D]
    1: StubFrame [pc: 00007FF741BA3810]
Security context: 0x008aebb008d1 <JSObject>
    2: save(aka save) [000002E34C3A6371] [BLAHBLAH\node_modules\@babel\register\lib\cache.js:52] [bytecode=000002D36A7A9BC9 offset=68](this=0x0025696c0471 <undefined>)
    3: processTicksAndRejections [0000018D77E422E1] [internal/process/task_queues.js:77] [bytecode=000002D36A7A8FA9 offset=73](this=0x018d77e41ed1 <process map = 00000229CDCF3931>)
    4: InternalFrame [pc: 00007FF741AC0DDE]
    5: EntryFrame [pc: 00007FF741AC09CC]

==== Details ================================================

[0]: ExitFrame [pc: 00007FF741B2CF5D]
[1]: StubFrame [pc: 00007FF741BA3810]
[2]: save(aka save) [000002E34C3A6371] [BLAHBLAH\node_modules\@babel\register\lib\cache.js:52] [bytecode=000002D36A7A9BC9 offset=68](this=0x0025696c0471 <undefined>) {
  // expression stack (top to bottom)
  [06] : 0x00c4c5f0cd19 <String[#2]:   >
  [05] : 0x0025696c01b1 <null>
  [04] : 0x03b73e642d79 <Object map = 00000229CDCC4BF1>
  [03] : 0x0025696c0541 <the_hole>
  [02] : 0x02e34c3a8221 <FunctionContext[11]>
  [01] : 0x02c30bd18721 <CatchContext[3]>
  [00] : 0x03b59f44b141 <String[#2]: {}>
--------- s o u r c e   c o d e ---------
function save() {\x0a  if (isCacheDisabled()) return;\x0a  let serialised = "{}";\x0a\x0a  try {\x0a    serialised = JSON.stringify(data, null, "  ");\x0a  } catch (err) {\x0a    if (err.message === "Invalid string length") {\x0a      e
rr.message = "Cache too large so it's been cleared.";\x0a      console.error(err.stack);\x0a    } else {\x0a  ...

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51353/

@jlennox
Copy link
Contributor Author

jlennox commented Feb 23, 2022

@nicolo-ribaudo Is there anything else I need to do prior to merge? Sorry -- I'm not familiar with the process here.

Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Another idea here: We currently set sourceMaps to "both", which creates an map object and embeds an inline sourceMappingUrl=${JSON.stringify(map)} in the code string. I'm not certain we need the object, we can parse it out of the code as needed (if we even need it, source-map-support supports just the inline comment without us doing anything)

@nicolo-ribaudo
Copy link
Member

I'm merging this for now, but I'm definitely open to further improvements.

@nicolo-ribaudo nicolo-ribaudo merged commit 9d31956 into babel:main Feb 24, 2022
@nicolo-ribaudo nicolo-ribaudo changed the title Remove pretty printing from register cache (5667) Remove pretty printing from register cache Feb 24, 2022
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 27, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants