-
Notifications
You must be signed in to change notification settings - Fork 123
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
Possible memory leak? #488
Comments
Thanks for submitting this issue @saul-jb . I will take a look here today. |
Hey Saul, I looked into this a bit and can't seem to find any memory leaks, but one thing that may be unexpected for this particular issue is that Helia starts up libp2p, and that's running in the background, creating dial processes and other things that will show more memory use than just using unixfs directly. I believe this increase in memory usage is coming from libp2p's processes. We could start up a separate libp2p node just to see what the baseline for libp2p consumption is to confirm further, but I'm pretty confident there's no issue and it's just a misunderstanding of the processes happening in the background. Some investigation belowInvestigating not starting helia & libp2p:With the below, all I do is set
diffdiff --git a/src/import.ts b/src/import.ts
index b314212..d9081ee 100644
--- a/src/import.ts
+++ b/src/import.ts
@@ -33,23 +33,36 @@ await createFile(filePath, 10 ** 9)
const helia = await createHelia({
blockstore: new FsBlockstore(Path.join(testDir, 'helia-blockstore')),
- datastore: new FsDatastore(Path.join(testDir, 'helia-datastore'))
+ datastore: new FsDatastore(Path.join(testDir, 'helia-datastore')),
+ start: false
})
const ufs = unixfs(helia)
+if (global.gc != null) {
+ console.log('garbage collection is enabled')
+} else {
+ console.log('garbage collection is disabled')
+}
+
+global.gc?.()
+
console.log('memory usage (baseline):', process.memoryUsage())
await all(ufs.addAll(globSource(testDir, '*'), { chunker: fixedSize() }))
+global.gc?.()
+
console.log('memory usage (after import):', process.memoryUsage())
await new Promise(resolve => setTimeout(resolve, 30000))
+global.gc?.()
+
console.log('memory usage (after waiting):', process.memoryUsage())
await helia.stop()
await new Promise(resolve => setTimeout(resolve, 2000))
-await fs.rm(testDir, { recursive: true })
+await fs.rm(testDir, { recursive: true, force: true }) use
|
I've done more testing and it seems that the issue is now fixed after the latest package releases, I couldn't find what package/update was responsible for fixing this. (The package versions in the repo I linked still have the issue.) I don't think that libp2p was to blame because the increase in memory was much to large for Libp2p and correlated with the file size. Regardless, thanks for taking the time to look into this. |
I have found that there seems to be far more memory allocation in Helia than what is expected when persistent stores are used, the allocation seems to be about equal with the import size so I think it is not freeing up buffer memory somewhere.
I have found 2 cases where this happens - importing local data into Helia & fetching remote data into Helia. The following examples show this first case.
This allocates excess memory:
This does not allocate excess memory:
Is this a mistake on my part, an expected result or a memory leak?
To make it easy to replicate I have made a repo that illustrates these examples (at least on my machine): https://github.com/saul-jb/helia-memory-leak
I have tested it with both node
v18.18.2
&v20.7.0
.As far as I can tell from my own debugging so far the problem lies somewhere in Helia's
BlockStorage
.The text was updated successfully, but these errors were encountered: