Remove ModTime check during build (#5125) #5351
Conversation
|
I don't think the windows integration test failure is correct, looks like it failed in the dependencies section. |
|
I would prefer to augment rather than remove the modtime check. I would want the heuristics to be something like:
That should bypass most of the slowdown potentially instituted by making the digest checks only occur (1) on creating the cache and (2) when the file mod times are incorrect |
|
I worked with @aschmois on this. I expected there to be a slowdown from replacing the modification time check with a digest comparison. I was surprised to find that if there was a difference, it was lost in the noise (at least for our project). Is there a large project we could test the relative performance with? Conceptually I like the idea of only relying on the file's contents rather than its metadata. But also of course a nice concept isn't any good if it's too slow. |
|
I would test on a monorepo like yesodweb/yesod. But even if testing shows no major performance impact, I'd still be worried that some users, based on type of hard drive, file system, cache settings, etc, would end up having a negative impact. |
|
I can try and test on different configurations and post the findings, I have a few different devices I can try to build on. |
|
With the help of @gera-cameron I ran the tests below, please let me know if something looks off about testing methods. AWS EC2 TestingRan on ec2 instance types
https://aws.amazon.com/ec2/instance-types/ Setup:$ curl -sSL https://get.haskellstack.org/ | sh # install 2.3.1
$ git clone --recurse-submodules http://github.com/yesodweb/yesod
$ cd yesod && stack test && cd - # download ghc and build dependencies
$ git clone http://github.com/aschmois/stack # clone PR
$ cd stack && stack test --copy-bins --local-bin-path bin --ghc-options '-O2' && cd - # build stack and copy binary into ./bin/
$ cp stack/bin/stack yesod/stackx && chmod u+x yesod/stackx # copy modified stack binary as stackx and make it executable
$ cd stack && git checkout b5d30906ebee25df1f2532255e245d329083b623 && stack test --copy-bins --local-bin-path bin --ghc-options '-O2' && cd - # build unmodified stack
$ cp stack/bin/stack yesod/stackz && chmod u+x yesod/stackz # copy unmodified stack binary as stackx and make it executableTest 1Each stack binary test started from cold boot then run sequentially. We don't expect numbers to change wildly here since they all start from a clean install Yesod Build Stack unmodified: $ stack clean --full && TIMEFORMAT='%6R'; time ./stackz buildYesod Build Stack modified: $ stack clean --full && TIMEFORMAT='%6R'; time ./stackx buildResultsAll results are in seconds
SummaryAll of these numbers are within margin of error of each other and we can assume that the same process is happening on each build. This is because digest is being calculated every time since the project was cleaned. Test 2This is where we expect things to show differences since no stack clean is done after the first one. Yesod Build Stack unmodified: $ stack clean --full && ./stackz build
$ export TIMEFORMAT='%6R'
$ time ./stackz build
$ time ./stackz build
$ time ./stackz buildYesod Build Stack modified: $ stack clean --full && ./stackx build
$ export TIMEFORMAT='%6R'
$ time ./stackx build
$ time ./stackx build
$ time ./stackx buildResultsAll results are in seconds
SummaryWe notice a ~21ms difference in magnetic drives calculating digests on a very restricted machine and ~11ms difference in ssd drives. Based off these results I think we should only use digests since mod times can bring more bugs (such as the CI one I ran into) and performance does not seem to be majorly affected. |
|
Based on the massive communication misconnect here, I think there's a fundamental misunderstanding here. I think I understand it, but before merging, let's confirm. I said above:
followed by:
Given that despite these comments, you've moved ahead with a whole bunch of performance testing, I think you're trying to imply:
Am I reading this conversation correctly? |
|
We don't think you're wrong, and we're not trying to overwhelm you with benchmarks. Like you, we expected the performance of this digest-based approach to be worse than the current approach based on file modification times. After working on this patch we were pleasantly surprised to find that it made effectively no impact whatsoever for our use case. Since your primary concern appeared to be performance, we tried to run a benchmark where we stacked the deck against ourselves: Large project, shared hosting, under powered machine, and spinning disks. Even with those unfavorable conditions we only saw about a 20 ms penalty, which is about 3% of the build time. Augmenting is very much an option. However we thought that the approach presented in this PR is both conceptually simpler and results in code that's easier to read, so we figured it was worth a shot. If you're saying that this approach is dead in the water even if the performance impact is minimal, fine. |
|
Sorry, I didn't mean to imply you're overwhelming me, I just meant that the evidence is overwhelmingly in favor of what you're saying. I am slightly concerned still, but I'm willing to take this as-is and see if anything complains about their hard drives thrashing later. Thank you! |
|
Sorry, just one more request: can you update the ChangeLog? |
|
I'd like to try to smooth out the situation, in no way do I want to attack anyone nor do any harm to this code base. I apologise if I came out like that, I can be a little cold sometimes when discussing code. I thought making fancy benchmarks would make a better case for the code written not worse. With that out of the way if we are moving forward with this I'd like to see if removing the mod time and size (if we don't need it as part of the digest check) is also something you think can be done I'd like to do so to cleanup the tuple composing. An afterthought, augmenting mod time does seem to be the best option in terms of safety but I think after looking at the benchmarks being more data safe can avoid some future bugs. I've always seen horror stories around mod time checks for caching and have honestly been battling wiht this bug for over a year; not directly but it has been at the back of my mind for that long 😅 . I really want what's best for stack not any random code! Please let me know what you think. |
|
Really, honestly, nothing to apologize for. I just wanted to hone in on whether there was a correctness issue I was missing, or if there was a different reason. Taylor clarified the situation to my satisfaction in the comment above. IIUC, we're not using the filesize or timestamp at all, so please do feel free to update the PR by removing them. I'm also not a fan of anonymous tuples in general, so either removing the multiple-data-returns or creating a custom datatype if they are needed would be great. Thanks! |
As the issue (#5125) suggests, modified time makes using a stack cache in CI very difficult (if at all possible). This PR suggests making a change in how the build process marks a file as dirty.
Summary of changes:
Questions and comments:
FileCacheInfo.