lib: allow CJS source map cache to be reclaimed#51711
Conversation
|
Review requested:
|
legendecas
force-pushed
the
source-maps/cjs
branch
from
4d27198 to
af592c5
Compare
legendecas
force-pushed
the
source-maps/cjs
branch
from
44c40aa to
5baf322
Compare
|
@aduh95 @joyeecheung would you mind taking a look again? Thank you! |
| run(); | ||
|
|
||
| // Run until the source map is cleared by GC, or fail the test after determined iterations. | ||
| common.gcUntil('SourceMap of deleted CJS module is cleared', () => { |
There was a problem hiding this comment.
I think this has the potential of flakes because the strategy used by common.gcUntil() isn't very sound (i.e. sometimes gc() is not aggressive enough to purge things). It would be safer to do it for a bunch more modules (say 30), and checks if any of them can be collected, which reduces the likelihood of the one single module that we are looking at just cannot be garbage collected in time because V8 just doesn't feel pressured enough to do that.
There was a problem hiding this comment.
Thanks for the suggestion, updated the test with more GC pressure to reduce the chance of flaky.
There was a problem hiding this comment.
Maybe we should launch a stress-test CI to make sure we're not introducing any flakiness
legendecas
force-pushed
the
source-maps/cjs
branch
2 times, most recently
from
9482198 to
611d7db
Compare
github-actions
Bot
removed
the
request-ci
|
@joyeecheung would you mind taking another look at this? Thank you! |
joyeecheung
left a comment
joyeecheung
left a comment
There was a problem hiding this comment.
LGTM but this needs a rebase
Unifies the CJS and ESM source map cache map with SourceMapCacheMap and allows the CJS cache entries to be queried more efficiently with a source url without iteration on an IterableWeakMap. Add a test to verify that the CJS source map cache entry can be reclaimed.
legendecas
force-pushed
the
source-maps/cjs
branch
from
611d7db to
ed1a8c7
Compare
github-actions
Bot
removed
the
request-ci
|
CI is green. @joyeecheung @aduh95 would be great to have another review, thanks! |
| run(); | ||
|
|
||
| // Run until the source map is cleared by GC, or fail the test after determined iterations. | ||
| common.gcUntil('SourceMap of deleted CJS module is cleared', () => { |
There was a problem hiding this comment.
Maybe we should launch a stress-test CI to make sure we're not introducing any flakiness
|
Stress test: https://ci.nodejs.org/job/node-stress-single-test/508 (passed) |
legendecas
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Backport in #56927 |
5 participants
Unifies the CJS and ESM source map cache map and allows the CJS cache
entries to be queried more efficiently with a source url without
iteration on an IterableWeakMap. Reclaims the CJS cache entry with
a FinalizationRegistry instead.
Add a regression test to verify that the CJS source map cache entry can be
reclaimed.