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

Add an option to debug Dune's metabolism #4412

Merged
1 commit merged into from Mar 30, 2021
Merged

Add an option to debug Dune's metabolism #4412

1 commit merged into from Mar 30, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 29, 2021

This PR add an option --debug-digests that makes Dune explain why it decides to re-digest some files:

$ dune build x --debug-digests
Re-digested file x because its stats changed:
{ old_digest = digest "b83631c134a9649ec383d0eb9c356803"
; new_digest = digest "705e0d7e5030b1831b18211b1e398faf"
; old_stats = { mtime = 1617030769.38; size = 0; perm = 436 }
; new_stats = { mtime = 1617030769.41; size = 2; perm = 436 }
}

I've sometimes wondered why Dune was re-digesting some files. This should help understand why.

@ghost ghost requested review from aalekseyev, rgrinberg and snowleopard and removed request for aalekseyev and rgrinberg March 29, 2021 15:14
Copy link
Collaborator

@snowleopard snowleopard left a comment

Choose a reason for hiding this comment

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

Looks good! Dune's metabolism should better be kept in check.

@snowleopard
Copy link
Collaborator

Seems to be flaky though -- in one of the test runs the change was apparently unnoticed?

@aalekseyev
Copy link
Collaborator

Could it be caused by low filesystem time resolution? I thought we had a fix for that.

@snowleopard
Copy link
Collaborator

Could it be caused by low filesystem time resolution? I thought we had a fix for that.

Not sure it's only about time resolution: if I understand correctly, the file size should have changed too. It seems like we're just observing stale results on the second run.

@ghost
Copy link
Author

ghost commented Mar 30, 2021

Alright, back to CI debugging 😭

@ghost
Copy link
Author

ghost commented Mar 30, 2021

  $ touch x
  $ dune build x
  $ dune internal dump _build/.digest-db
  { checked_key = 0; max_timestamp = 1617097739.; table = map {} }

🤔

@ghost
Copy link
Author

ghost commented Mar 30, 2021

Oh, I think I got it. I accidentally removed a call to set_max_timestamp

@ghost
Copy link
Author

ghost commented Mar 30, 2021

Interesting: I added a debug print for when we drop entries because their mtime is the same as the fs clock, and it appears that we drop on Linux as well, which I didn't expect. The entry in question was _build/default/x.

@ghost
Copy link
Author

ghost commented Mar 30, 2021

So here we were effectively dropping the entry for In_source_tree "x" during the first build. I adapted the test to be less racy.

@ghost
Copy link
Author

ghost commented Mar 30, 2021

I have already learned something with this option: in the digest cache, we save a reduced version of the file stats to detect when the file changes. However, when we do that for targets we use the stats before removing the write permissions, which means that during the second build we always re-hash everything 🤦 Writing a PR to fix this.

Signed-off-by: Jeremie Dimino <[email protected]>
@ghost ghost merged commit eaedc6a into ocaml:main Mar 30, 2021
@ghost
Copy link
Author

ghost commented Mar 30, 2021

Fixed in #4419. @aalekseyev I also ended up adding this option to wait for the FS clock as there was too much noise.

This pull request was closed.
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