fix: use lru-cache for packuments #7463
Conversation
|
In a sample size of 1 this makes the example in #7276 not crash node when calling it with |
Somewhat related to #7276 and #7463. I don't think there is a reason to cache the promise here. And if we ever did choose to replace this with an LRUCache we would need to know the size of what we are caching which will be easier if we only cache the resulting manifest. I also added a comment about why I think we are removing the license from manifests here. License was removed in #7126 which looks to be a purposeful change but I could not find a reason. Adding the license back in causes many snapshots to fail because the license is now present in lockfiles, so that's how I came up with the comment.
|
|
|
Packument size to memory ratio (units are 1000 not 1024): The current tradeoff here is that we are swapping memory for disk reads (the cache reads from disk). On modern machines with large amounts of memory this is likely not a very big tradeoff, most high-memory high-end systems use ssd disks. This is more impactful of low memory systems, where running out of memory means the process dies. I suggest an initial limit of 1M for the maximum packument we'll put in the cache, and then we use a multiplier of .6 for all packuments when reporting their size to lru-cache. This will over-report smaller packuments in favor of not accidentally under reporting medium sized ones an accidentally going further over the desired memory usage. I don't have a good way to calculate what percentage of the |
Somewhat related to #7276 and #7463. I don't think there is a reason to cache the promise here. And if we ever did choose to replace this with an LRUCache we would need to know the size of what we are caching which will be easier if we only cache the resulting manifest. I also added a comment about why I think we are removing the license from manifests here. License was removed in #7126 which looks to be a purposeful change but I could not find a reason. Adding the license back in causes many snapshots to fail because the license is now present in lockfiles, so that's how I came up with the comment.
|
This is ready for tweaking based on your findings @wraithgar. I was able to get all the tests passing with the new packument cache in place. The only changes were to logging and a few tests with the mock-registry needed |
|
@wraithgar Approved and ready to land I think. Can you squash with an appropriate body message that closes #7276? |
|
npm/pacote#369 is still an issue but this doesn't make that issue any worse so it shouldn't block this PR |
This adds a new packument cache that is an instance of `lru-cache`. It uses that package's ability to limit content based on size, and has some multipliers based on research to mostly correctly approximate the correlation between packument size and its memory usage. It also limits the total size of the cache based on the actual heap available. Closes: #7276 Related: npm/pacote#369
|
Here is the research used to calculate the multipliers. |
We could of course also try to increase the default node memory limit? Or is that a bad idea. |
That's up to you. This was built to react to that value, so scaling it would mean npm using more memory in this cache. It would take doing some benchmarking but I suspect that on modern systems w/ lots of memory and ssd drives the tradeoff isn't very large. |
Fixes #7276