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

FileMonitor features #3129

Closed
wants to merge 6 commits into from
Closed

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented Feb 10, 2016

Two new file monitor features, one for accuracy and one for performance.

Edsko reported (but no ticket) that edits to files during a build were not caught, and so the (new-)build command thought everything was now up to date when in fact it was not. The problem was that we take the file snapshot after the build completes and so obviously we miss changes during the build. Doing the snapshot before would be the obvious thing but in general it's hard to know in advance all the files. It's easier to find out the files as we go so we only know the full set at the end. This is certainly the pattern that the RebuildMonad follows.

The solution is quite simple and general: take a timestamp at the start of the build (or in general any action using the FileMonitor) and then when we take the snapshot at the end, any file that changed after we took the timestamp we can consider to have already changed. So we just record in the state that it already changed (we use the Nothing case of a Maybe ModTime for this purpose).

The performance one is simply that we were re-hashing all the files, even the ones that had not changed. Reusing the previous state as a cache is simple and effective.

Verified

This commit was signed with the committer’s verified signature.
0x2142 Matt Schmitz
Change the representation of files that have already changed by the time
we take the snapshot. We had two extra constructor, but now instead we
represent it with the normal constructors but with a Maybe ModTime. The
reson for this change is that it's easier to extend to the globbing case,
and we will shortly have a case there where we also need to be able to
represent files that have already changed.

So this is really just a small refactoring to make the next change
easier.

Verified

This commit was signed with the committer’s verified signature.
0x2142 Matt Schmitz
If we take the snapshot after the action has completed then we have a
problem. The problem is that files might have changed while the action
was running but /after/ the action read them. If we take the snapshot
after the action completes then we will miss these changes.

The solution is to record a timestamp before beginning execution of the
action and then we make the conservative assumption that any file that
has changed since then has already changed, ie the file monitor state
for these files will be such that checkFileMonitorChanged will report
that they have changed.

Makes use of this in the Rebuild monad so everything using this will get
the feature for free. Also adds a test.

Verified

This commit was signed with the committer’s verified signature.
0x2142 Matt Schmitz
We know the previous mtime and content hash for all the files in the
previous file set, so reuse these content hashes when we determine they
are still valid (which is when the mtime is the same). This saves
re-hashing files that have not changed.

No test for this as such since it's a performance rather than
correctness thing, but checked ad-hoc that the cache does get a perfect
hit rate.

Fixes one of two parts of issue haskell#3019.
@23Skidoo
Copy link
Member

Not sure why the Travis build didn't start. @dcoutts, can you please tweak something and then do a commit and a push (or force-push)?

@@ -165,16 +172,16 @@ type ModTime = ClockTime
-- This covers all the cases of 'MonitorFilePath' except for globs which is
-- covered separately by 'MonitorStateGlob'.
--
-- The @Maybe ModTime@ is to cover the case where the we already consider the
Copy link
Member

Choose a reason for hiding this comment

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

s/the we/we/

@23Skidoo
Copy link
Member

LGTM modulo minor comments.

@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 10, 2016

Ok thanks, will do. Yes I was also wondering why the travis build didn't start.

@dcoutts dcoutts mentioned this pull request Feb 16, 2016
@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 16, 2016

@23Skidoo I'll get back to this while you look at #3156, then after that will be more new code...

@23Skidoo
Copy link
Member

@dcoutts Sure. Have you received my mail re: release planning?

@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 16, 2016

@23Skidoo yes, I replied earlier today. Sounds sensible.

@23Skidoo
Copy link
Member

@dcoutts Looks like I didn't get your mail for some reason.

Verified

This commit was signed with the committer’s verified signature.
0x2142 Matt Schmitz
@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 17, 2016

@23Skidoo hmm, odd. I checked I did send it, but yes the answer is yes that sounds fine.

So is this ok to go in now? I'll squash the fixup patches.

@23Skidoo
Copy link
Member

Yes, let's merge.

@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 18, 2016

Ok, thanks, will do.

@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 19, 2016

Merged bed80a8.

@dcoutts dcoutts closed this Feb 19, 2016
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.

None yet

2 participants