fix(tarball): use rwlock instead of dashmap to avoid potential deadlock #200
Conversation
await-ovo
force-pushed
the
fix-mem-cache-deadlock
branch
from
0d594da
to
ac2c01c
Compare
Yes, I'll try to add the related test ~ |
await-ovo
force-pushed
the
fix-mem-cache-deadlock
branch
from
bff8d05
to
0471299
Compare
| eprintln!("Creating package.json..."); | ||
| let manifest_path = workspace.join("package.json"); | ||
| let package_json_content = serde_json::json!({ | ||
| "dependencies": { |
There was a problem hiding this comment.
Before rebase the branch main, i.e. without mock-registry, the test passed, but now it doesn't seem to be able to install express, so it will take a bit more time to see what's going on.
| @@ -88,7 +87,7 @@ pub enum CacheValue { | |||
| /// Internal in-memory cache of tarballs. | |||
| /// | |||
| /// The key of this hashmap is the url of each tarball. | |||
| pub type MemCache = DashMap<String, Arc<RwLock<CacheValue>>>; | |||
| pub type MemCache = RwLock<HashMap<String, Arc<RwLock<CacheValue>>>>; | |||
There was a problem hiding this comment.
I wonder if MemoMap can be sent across async? Since the cache is growth only.
There was a problem hiding this comment.
Looks like it is perfect for our scene. I just tried memo-map on this branch, and it works well.
I've also tried integrated-benchmark on the branch main, fix-mem-cache-deadlock, and refactor-use-memo-map, here is the result on my local machine:
- fronzen-lockfile
$ just integrated-benchmark fix-mem-cache-deadlock refactor-use-memo-map main -s=frozen-lockfile
Benchmark 1: pacquet@fix-mem-cache-deadlock
Time (mean ± σ): 532.3 ms ± 189.3 ms [User: 99.6 ms, System: 729.0 ms]
Range (min … max): 390.8 ms … 1049.7 ms 10 runs
Warning: The first benchmarking run for this command was significantly slower than the rest (1.050 s). This could be caused by (filesystem) caches that were not filled until after the first run. You are already using both the '--warmup' option as well as the '--prepare' option. Consider re-running the benchmark on a quiet system. Maybe it was a random outlier. Alternatively, consider increasing the warmup count.
Benchmark 2: pacquet@refactor-use-memo-map
Time (mean ± σ): 517.7 ms ± 104.7 ms [User: 110.5 ms, System: 764.0 ms]
Range (min … max): 412.1 ms … 707.7 ms 10 runs
Benchmark 3: pacquet@main
Time (mean ± σ): 517.9 ms ± 95.5 ms [User: 113.1 ms, System: 801.1 ms]
Range (min … max): 410.9 ms … 712.6 ms 10 runs
Summary
pacquet@refactor-use-memo-map ran
1.00 ± 0.27 times faster than pacquet@main
1.03 ± 0.42 times faster than pacquet@fix-mem-cache-deadlock
- clean-install
$ just integrated-benchmark fix-mem-cache-deadlock refactor-use-memo-map main -s=clean-install
Benchmark 1: pacquet@fix-mem-cache-deadlock
Time (mean ± σ): 7.350 s ± 0.156 s [User: 0.587 s, System: 0.909 s]
Range (min … max): 7.092 s … 7.603 s 10 runs
Benchmark 2: pacquet@refactor-use-memo-map
Time (mean ± σ): 7.124 s ± 0.849 s [User: 0.521 s, System: 0.868 s]
Range (min … max): 6.713 s … 9.532 s 10 runs
Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs.
Benchmark 3: pacquet@main
⠦ Initial time measurement ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ ETA 00:00:00^C%
$ just integrated-benchmark fix-mem-cache-deadlock refactor-use-memo-map -s=clean-install
Building "refactor-use-memo-map"...
Finished release [optimized] target(s) in 0.57s
Benchmark 1: pacquet@fix-mem-cache-deadlock
Time (mean ± σ): 7.400 s ± 0.157 s [User: 0.578 s, System: 0.936 s]
Range (min … max): 7.207 s … 7.761 s 10 runs
Benchmark 2: pacquet@refactor-use-memo-map
Time (mean ± σ): 7.706 s ± 1.574 s [User: 0.544 s, System: 0.910 s]
Range (min … max): 6.906 s … 12.145 s 10 runs
Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs.
Summary
pacquet@fix-mem-cache-deadlock ran
1.04 ± 0.21 times faster than pacquet@refactor-use-memo-map
It seems that integrated-benchmark on the branch main will get a deadlock currently, so there's no comparison to the main branch.
Overall, thanks for your suggestion, it would be better to use I don't know if we should use memo-map, If there's no problem on your end, I think we can just use it ~memo-map here, it looks like RwLock for clean-install would be a little better?
There was a problem hiding this comment.
Frozen lockfile doesn't use memory cache so it's irrelevant (It invokes run_without_mem_cache).
I don't know if we should use
memo-maphere, it looks likeRwLockfor clean-install would be a little better?
The integrated-benchmark script has a --fixture-dir/-D option, you may use it to specify a bigger package.json file.
There was a problem hiding this comment.
for this package.json:
{
"dependencies": {
"nanoid": "^3.3.6",
"picocolors": "^1.0.0",
"source-map-js": "^1.0.2"
},
"devDependencies": {
"@size-limit/preset-small-lib": "^9.0.0",
"@types/node": "^20.8.3",
"c8": "^8.0.1",
"check-dts": "^0.7.2",
"clean-publish": "^4.2.0",
"concat-with-sourcemaps": "^1.1.0",
"nanodelay": "^1.0.8",
"nanospy": "^1.0.0",
"postcss-parser-tests": "^8.8.0",
"simple-git-hooks": "^2.9.0",
"size-limit": "^9.0.0",
"strip-ansi": "^6.0.1",
"ts-node": "^10.9.1",
"typescript": "^5.2.2",
"uvu": "^0.5.6"
}
}
Run just integrated-benchmark fix-mem-cache-deadlock refactor-use-memo-map -s=clean-install -D=/Users/await-ovo/Documents/test/test-pnpm/alotta-modules show that:
List of branches:
Building "refactor-use-memo-map"...
Finished release [optimized] target(s) in 0.47s
Benchmark 1: pacquet@fix-mem-cache-deadlock
Time (mean ± σ): 3.943 s ± 0.205 s [User: 3.193 s, System: 2.717 s]
Range (min … max): 3.618 s … 4.192 s 10 runs
Benchmark 2: pacquet@refactor-use-memo-map
Time (mean ± σ): 4.041 s ± 0.184 s [User: 3.328 s, System: 2.843 s]
Range (min … max): 3.818 s … 4.355 s 10 runs
Summary
pacquet@fix-mem-cache-deadlock ran
1.02 ± 0.07 times faster than pacquet@refactor-use-memo-map
There was a problem hiding this comment.
I want to use this package.json to do a benchmark, but there will be a Too many open files error during install.
There was a problem hiding this comment.
Try a smaller one, for example:
{
"dependencies": {
"@babel/core": "^7.23.2",
"@tsfun/all": "0.0.37",
"@types/node": "16.18.39",
"cross-env": "7.0.3",
"glob-parent": "6.0.2",
"husky": "8.0.3",
"jest": "29.7.0",
"json5": "2.2.3",
"jsonwebtoken": "9.0.2",
"keyv": "4.5.3",
"react": "18.2.0",
"rimraf": "3.0.2",
"semver": "7.5.4",
"shx": "0.3.4",
"ts-jest": "29.1.1",
"ts-node": "10.9.1",
"typescript": "5.2.2",
"verdaccio": "5.27.0",
"yaml": "2.3.4"
}
}There was a problem hiding this comment.
Unfortunately, this will get an error:
Maybe we should test the benchmark with a bigger repository after this is resolved.
There was a problem hiding this comment.
Can you try combine your branches with #210 into 2 new branches and see if the "too many open files" error still exist? If the error disappear, can you try benchmark them?
There was a problem hiding this comment.
I tried, but I still can't get the results of integrated-benchmark, it seems to be because of a circular dependency in alotta-files?
There was a problem hiding this comment.
@zkochan It seems pacquet can't yet handle circular dependencies (without lockfile). Can you tell us which is the circular dependencies to remove?
Also, @await-ovo, another PR was just merged, you should pull from upstream or rebase.
Let's say we have a project that has
expressin dependencies and devDependencies:Running
pacquet/target/debug/pacquet installover and over again under this project sometimes hangs forever.Inspect thread's backtrace using rust-lldb you can see that there seems to be a deadlock in the dashmap:
It looks like dashmap doesn't recommend keeping references across
.await, which should also be the cause of the deadlock here, if I understand correctly.Since I'm new to Rust, I'm not sure if there's a better way to solve this problem, I tried replacing the dashmap with
RwLocklike in this PR and it seems to work fine.