Add an option to control progress reporting #1513
Conversation
|
I think the O(log n) is easily fixable on the Shake side. If it was fixed, would you still want to make this change? When you say "quite expensive" do you have any numbers you can share? |
Yes, this change still makes sense, to remove the arbitrary exclusion of The icicle graph below is the result of profiling an "edit" experiment over a project with ~40k source files where there is only a FOI which only has 3 transitive dependencies. It shows that ~50% of the time spent in
|
|
Ouch, I'll get a constant time actionBracket done today with luck... |
| import Bag (listToBag) | ||
| import ErrUtils (mkErrMsg) | ||
| import Outputable (text, neverQualify) | ||
| #endif |
There was a problem hiding this comment.
Why did this #if disappear? Seems unrelated.
There was a problem hiding this comment.
doh, again! It's the second time it happens to me, I think this could be a problem with the pre-commit hook, /cc @Ailrun
There was a problem hiding this comment.
Could be... It's probably because CPP support for the formatter is not so reliable. Unfortunately, I don't know any formatter that works well with CPP...
| } | ||
|
|
||
| defaultSkipProgress :: Typeable a => a -> Bool | ||
| defaultSkipProgress key = case () of |
There was a problem hiding this comment.
Could we comment as to why these nodes get excluded? The original reason for excluding GetFileExists was that given a module Foo.hs, we might have N GetFileExists at each include path, at .lhs, at .hs-boot etc. Whereas every other rule is pretty much only at a single filepath. Is that property true for modtime? It definitely isn't for getfilecontents, but is that about performance?
There was a problem hiding this comment.
It's performance, but I haven't measured. I'll add a comment or revert to just GetFileExists
There was a problem hiding this comment.
I don't think is unreasonable - its a commonly called thing that is mostly irrelevant for performance concerns. I do think its important to say that GetFileExists is specially, because its not obvious that is has that weird behaviour, and how if you do include GetFileExists your progress is basically meaningless.
pepeiborra
force-pushed
the
optionalProgressReporting
branch
from
dc0966e
to
0d9df1b
Compare
The current implementation of progress reporting uses
actionBracketwhich has O(logC) complexity in the number of registered Cleanups (ndmitchell/shake#797).Since we do progress reporting for every File and Rule updated, the worst case complexity is around O(FRlogC) for all the files in the project. In large projects, this is quite expensive.
Since
otTracedActionis implemented usingactionBrackettoo, I have made it conditional on eventlogs being enabled.For reviewers understanding: I had to move some types around to avoid cyclic module dependencies.