make nix detection faster #2113
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes Nix package detection by parallelizing nix-store calls, reducing detection time from ~120ms to ~80ms on macOS. The implementation also removes the hash-based cache mechanism which could cause performance regressions on cache misses.
Key Changes:
- Introduces parallel execution of
nix-storequeries usinggetNixPackagesMultiImplfunction - Adds a macro-based API
ffPackagesGetNixMultifor convenient multi-path detection - Migrates macOS implementation to use the new parallel API
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/detection/packages/packages_nix.c | Implements parallel Nix package detection with getNixPackagesMultiImpl function that spawns multiple nix-store processes concurrently |
| src/detection/packages/packages_apple.c | Migrates from sequential to parallel Nix detection API, collecting all three Nix paths and calling them in parallel |
| src/detection/packages/packages.h | Adds function declaration for getNixPackagesMultiImpl and convenience macro ffPackagesGetNixMulti for parallel detection |
| }, false, &handle[i]); | ||
| } | ||
| } | ||
| for (uint8_t i = 0; i < length; i++){ |
There was a problem hiding this comment.
Missing space after closing brace in for loop. Should be for (uint8_t i = 0; i < length; i++) { to be consistent with the project's coding style.
| } | ||
| for (uint8_t i = 0; i < length; i++){ | ||
| FF_STRBUF_AUTO_DESTROY output = ffStrbufCreateA(1024); | ||
| if (!handle[i].pid){ |
There was a problem hiding this comment.
Incorrect spacing in if statement. Should have a space after 'if' to match the project's coding style. Should be if (!handle[i].pid) {.
| if (!handle[i].pid){ | |
| if (!handle[i].pid) { |
| counts[i] = 0; | ||
| continue; | ||
| } | ||
| ffProcessReadOutput(&handle[i], &output); |
There was a problem hiding this comment.
The return value of ffProcessReadOutput is not checked for errors. According to the function signature in processing.h, this function returns a const char* error message that should be checked. If the process fails or returns an error, the output buffer may be incomplete or invalid, leading to incorrect package counts. The error should be checked and handled appropriately.
| ffProcessReadOutput(&handle[i], &output); | |
| const char* error = ffProcessReadOutput(&handle[i], &output); | |
| if (error != NULL) | |
| { | |
| counts[i] = 0; | |
| continue; | |
| } |
| ffProcessSpawn((char* const[]) { | ||
| "nix-store", | ||
| "--query", | ||
| "--requisites", | ||
| paths[i], | ||
| NULL | ||
| }, false, &handle[i]); |
There was a problem hiding this comment.
The return value of ffProcessSpawn is not checked for errors. According to the function signature in processing.h, this function returns a const char* error message that should be checked. If the process fails to spawn, handle[i].pid might still be set to a non-zero value (depending on implementation), leading to attempts to read output from a non-existent process. The error should be checked and if non-null, handle[i].pid should remain 0.
| ffProcessSpawn((char* const[]) { | |
| "nix-store", | |
| "--query", | |
| "--requisites", | |
| paths[i], | |
| NULL | |
| }, false, &handle[i]); | |
| const char* error = ffProcessSpawn((char* const[]) { | |
| "nix-store", | |
| "--query", | |
| "--requisites", | |
| paths[i], | |
| NULL | |
| }, false, &handle[i]); | |
| if (error != NULL) | |
| handle[i].pid = 0; |
| { | ||
| //Implementation based on bash script from here: | ||
| //https://github.com/fastfetch-cli/fastfetch/issues/195#issuecomment-1191748222 | ||
| // no need to use hash, it is not faster |
There was a problem hiding this comment.
Comment has inconsistent spacing. Should have two spaces after '//' to match the project's comment style as seen in line 62.
| // no need to use hash, it is not faster | |
| // no need to use hash, it is not faster |
| #define ffPackagesGetNixMulti(dirs, counts, N) \ | ||
| do { \ | ||
| FFProcessHandle _handles[N]; \ | ||
| char* _paths[N]; \ | ||
| for (uint8_t i = 0; i < N; i++) { \ | ||
| _paths[i] = dirs[i]->chars; \ | ||
| } \ |
There was a problem hiding this comment.
Inconsistent formatting with extra spaces. The macro definition should have consistent spacing. Consider aligning the backslashes at a consistent column or removing extra spaces between tokens.
| #define ffPackagesGetNixMulti(dirs, counts, N) \ | |
| do { \ | |
| FFProcessHandle _handles[N]; \ | |
| char* _paths[N]; \ | |
| for (uint8_t i = 0; i < N; i++) { \ | |
| _paths[i] = dirs[i]->chars; \ | |
| } \ | |
| #define ffPackagesGetNixMulti(dirs, counts, N) \ | |
| do { \ | |
| FFProcessHandle _handles[N]; \ | |
| char* _paths[N]; \ | |
| for (uint8_t i = 0; i < N; i++) { \ | |
| _paths[i] = dirs[i]->chars; \ | |
| } \ |
| //Implementation based on bash script from here: | ||
| //https://github.com/fastfetch-cli/fastfetch/issues/195#issuecomment-1191748222 | ||
| // no need to use hash, it is not faster | ||
| for (uint8_t i = 0; i < length; i++){ |
There was a problem hiding this comment.
Missing space after closing brace in for loop. Should be for (uint8_t i = 0; i < length; i++) { to be consistent with the project's coding style seen in line 73 and throughout the codebase.
| for (uint8_t i = 0; i < length; i++){ | ||
| handle[i].pid = 0; | ||
| // Nix detection is kinda slow, so we only do it if the dir exists | ||
| if(ffPathExists(paths[i], FF_PATHTYPE_DIRECTORY)){ |
There was a problem hiding this comment.
Incorrect spacing in if statement. Should have a space after 'if' to match the project's coding style as seen in line 75 and throughout the codebase. Should be if (ffPathExists(paths[i], FF_PATHTYPE_DIRECTORY)) {.
| if(ffPathExists(paths[i], FF_PATHTYPE_DIRECTORY)){ | |
| if (ffPathExists(paths[i], FF_PATHTYPE_DIRECTORY)) { |
2 participants
Counting nix-user and nix-default sequentially is quite slow.
On my MacBook, it takes around 120 ms (measured with
fastfetch --stat).This PR changes the implementation to call
nix-storein parallel, reducing the time to about 80 ms on my machine.It also removes the hash cache. In practice, the cache only improves performance by roughly 10 ms, while a cache miss (when the hash changes) can take up to 230 ms, making the overall behavior worse.