Skip to content
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

Improve remote store eval time #2334

Merged

Conversation

TeofilC
Copy link
Contributor

@TeofilC TeofilC commented Mar 19, 2025

This code used to put the .drv path into the string context for each package. That meant we had to query the store each time one of these was evaluated for the out path. This is pretty quick when using a non-remote store. But the latency with the remote store really slows us down.

We should just calculate the outPath once instead, and use that throughout.

@michaelpj
Copy link
Collaborator

Comment the reason so we don't regress it?

@TeofilC
Copy link
Contributor Author

TeofilC commented Mar 19, 2025

Actually I'm not sure if this fixes it. I'll mark it as draft for now

@TeofilC TeofilC marked this pull request as draft March 19, 2025 13:13
@TeofilC
Copy link
Contributor Author

TeofilC commented Mar 19, 2025

Actually I'm not even sure what I want it to do is possible. As far as I can tell, whatever I do it will create a string with a context depending on the derivation. And each version of this will rebuild the derivation when imported.

@TeofilC TeofilC force-pushed the teo/improve-remote-store-perf branch from 08166ec to 4de0315 Compare March 19, 2025 14:01
@TeofilC TeofilC marked this pull request as ready for review March 19, 2025 14:01
@TeofilC
Copy link
Contributor Author

TeofilC commented Mar 19, 2025

I've gotten it working now. We needed to do unsafeDiscardStringContext like we do elsewhere in the file. And I've added a comment

This code used to put the .drv path into the string context for each
package. That meant we had to query the store each time one of these was
evaluated for the out path. This is pretty quick when using a non-remote
store. But the latency with the remote store really slows us down.

We should just calculate the outPath once instead, and use that
throughout.
@TeofilC TeofilC force-pushed the teo/improve-remote-store-perf branch from 4de0315 to 4b7364b Compare March 19, 2025 14:02
@hamishmack hamishmack merged commit a96cf6c into input-output-hk:master Mar 19, 2025
7546 of 7548 checks passed
@TeofilC TeofilC deleted the teo/improve-remote-store-perf branch March 20, 2025 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants