fix: use the process cache to reduce the memory for asar file

[xuwanghu]

Description of Change

The memory leaks at /lib/asar/fs-wrapper.ts, const cachedArchives = new Map<string, NodeJS.AsarArchive>() Whenever a new worker is created, it will have an instance of this map. The contents of this map will not be cleaned up immediately.
And there is a process-level cache at /shell/common/asar/asar_util.cc, static base::NoDestructor s_archive_map
In summary, I decide to use this process-level cache to save the asar cache instead of creating a new one for each worker

Resolves #36597

Checklist

• PR description included and stakeholders cc'd
• npm test passes

Release Notes

Notes: use the process cache to reduce the memory for asar file

[BlackHole1]

It appears the relevant PR is #33216
/cc @indutny

[RaisinTen]

The change makes sense to me. Is there any chance you could add a test? Maybe by comparing the value of process.memoryUsage().heapUsed before and after the memory leak?

[RaisinTen]

@zcbenz could you also take a look?

[indutny]

Nice catch!

[xuwanghu]

The change makes sense to me. Is there any chance you could add a test? Maybe by comparing the value of process.memoryUsage().heapUsed before and after the memory leak?

I have test it in our app which is build with electron, the memory usage gap is determined by the size of the ASAR file and the number of workers used in the program. The orange line is on electron 17.0.0 and the blue line is after my repair.

[RaisinTen]

Right, I mean adding a test to spec to make sure that future changes don't regress on this test case.

[MarshallOfSound]

Haven't looked into it but are we sure that Archive and GetOrCreateAsarArchive are thread safe? Maybe we should look into cleaning up archives on thread destruction rather than re-using archives across threads if not.

[xuwanghu]

Haven't looked into it but are we sure that Archive and GetOrCreateAsarArchive are thread safe? Maybe we should look into cleaning up archives on thread destruction rather than re-using archives across threads if not.

I refer to #29292 , and my conclusion is that Archive and GetOrCreateAsarArchive are thread-safe

[xuwanghu]

Right, I mean adding a test to spec to make sure that future changes don't regress on this test case.

I will learn and try to add a test to the electron, but what about the data that determines a failed test? 1kb or 1mb? It is not easy to confirm the criteria for determining test failure. It is also difficult to confirm the time when the memory reclamation is completed when the worker exits

[RaisinTen]

Hmm, I'm not sure how this can be reproduced consistently. Maybe you would need to experiment with the values a bit? If it's hard to consistently reproduce, I guess it's okay to land this without a test too.

[nornagon]

asar::GetOrCreateAsarArchive is guarded by a lock, so I think this is safe:

electron/shell/common/asar/asar_util.cc

Lines 62 to 63 in 6a798b1

	std::shared_ptr<Archive> GetOrCreateAsarArchive(const base::FilePath& path) {
	base::AutoLock auto_lock(GetArchiveCacheLock());

[RaisinTen]

lgtm

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉

[release-clerk]

Release Notes Persisted

use the process cache to reduce the memory for asar file

[trop]

I have automatically backported this PR to "21-x-y", please check out #36663

[trop]

I have automatically backported this PR to "22-x-y", please check out #36664

[trop]

I have automatically backported this PR to "23-x-y", please check out #36665