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

lazily decode cache files for checking invalidation #7516

Merged
merged 6 commits into from
Aug 9, 2021

Conversation

gbaz
Copy link
Collaborator

@gbaz gbaz commented Aug 6, 2021

This yields a significant (15% ?) speedup on rebuilding build plans for projects with lots of individual cabal packages. (As in https://github.com/peterbecich/cabal-resolver-issue which is a repro for #7466). In such cases the cache files can grow quite large (up to approx 30M).

The way the old filemonitor / caching system worked, it would first read and fully parse each cache file, and then check it to see if it was invalidated (an operation that only involved the header portion at the beginning of the file). When files are small, the full parse isn't noticeable. But as files grow large, this parse can become quite expensive.

This changes the deserialization to go in two steps -- first parse the header info, and check for invalidation. Then, only if the cache is valid, proceed to deserialize the remaining (potentially large) serialized value. Testing reveals that this removes deserialization as a noticeable cost center immediately -- dropping it to about 3% of total time.

(note: the writing out of the cache files is still about 10% of total time, but that seems fairly unavoidable)

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 7, 2021

The failure is because the binary 7.6.3 ships with doesn't export runGetOrFail or any other incremental method I can see :-/

cf: https://downloads.haskell.org/~ghc/7.6.3/docs/html/libraries/binary-0.5.1.1/Data-Binary-Get.html

Any suggestions the best way to deal with this?

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 7, 2021

Oh, hrm. It has an incremental function runGetState but just no functions that don't throw on error... I suppose I can try to make do with that.

@fgaz
Copy link
Member

fgaz commented Aug 8, 2021

You could move structuredDecodeTriple to cabal-install, which does not support ghc 7.6

@gbaz
Copy link
Collaborator Author

gbaz commented Aug 8, 2021

Great suggestion @fgaz, done!

@Mikolaj Mikolaj self-requested a review August 9, 2021 06:46
@Mikolaj
Copy link
Member

Mikolaj commented Aug 9, 2021

I wonder if writing out of the cache files can be done on a separate (low priority) thread top speed things up.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've bitched about whitespace, but otherwise it looks great.

cabal-install/src/Distribution/Client/FileMonitor.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/FileMonitor.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/FileMonitor.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/FileMonitor.hs Outdated Show resolved Hide resolved
@gbaz
Copy link
Collaborator Author

gbaz commented Aug 9, 2021

I wonder if writing out of the cache files can be done on a separate (low priority) thread top speed things up.

I thought about that too. We don't really use separate threads for IO ops elsewhere in cabal afaik, so I didn't know about setting a precedent. I'd think we'd need some slightly careful architecture to make sure that reading and writing didn't step on one another, etc., so not sure about the tradeoff between complexity and a constant win in some large cases.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 9, 2021

Yes, also, a user trying to exit before writing is finished, while otherwise cabal seems to have done it's job and is just hanging, would deserve a warning and a confirmation, which again adds to complexity of the solution.

If cabal always keeps its caches in memory and so never reads caches that it writes in the same session, that would simplify things. OTOH, this makes cabal use more memory than it would otherwise.

BTW, the CI broke on some timeout, so I'd ignore it. (edit: and merge)

@gbaz gbaz merged commit 2bd0588 into master Aug 9, 2021
@gbaz gbaz deleted the gb/speed-cache-reading branch August 9, 2021 22:20
@emilypi
Copy link
Member

emilypi commented Aug 12, 2021

@Mergifyio backport 3.6

@mergify
Copy link
Contributor

mergify bot commented Aug 12, 2021

Command backport 3.6: success

Backports have been created

mergify bot added a commit that referenced this pull request Aug 12, 2021
…7537)

* lazily decode cache files for checking invalidation

(cherry picked from commit 3dcfe27)

* Update Structured.hs

(cherry picked from commit 5a4290c)

* move structuredDecodeTriple to cabal-install

(cherry picked from commit c1d5d4f)

* fix type signatures

(cherry picked from commit 7e30fd9)

# Conflicts:
#	cabal-install/src/Distribution/Client/FileMonitor.hs

* fix whitespace

(cherry picked from commit f8bdd7f)

* Update FileMonitor.hs

Co-authored-by: Gershom Bazerman <gershom@arista.com>
Co-authored-by: gbaz <gershomb@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants