search

resonai / ybt

Yet another Build Tool
Apache License 2.0
2 stars 2 forks source link

Check whether the `dirty` optimization is required #219

Open dana-resonai opened 2 years ago

dana-resonai commented 2 years ago

Before checking if a target is cached, we check whether one of its dependencies is dirty (meaning - was built in this run) and if it is we don't even check the cache and decide to build the target. This is an optimization because if a dependency was changed, its hash changed and the dependencies hash are included in the target's hash. So the target's hash changes and we will build it again anyway.

It doesn't seem like it should save too much time, and this optimization is making problems. So we suggest timing it to see if we can just remove it.

dana-resonai commented 2 years ago

I checked it with one of our big targets and got the following timings:

Running ybt build :target -j8 --logtostderr after rm -rf yabtwork: real 10m29.321s user 6m27.324s sys 0m16.721s

Remove the dirty check, running ybt build :target -j8 --logtostderr after rm -rf yabtwork: real 10m28.314s user 6m48.435s sys 0m17.097s

It is pretty much the same.

So if the only reason for this check is optimization seems like we can remove it.

itamaro commented 2 years ago

IIRC, the dirty thing was implemented before we had caching, so it was a significant optimization (maybe?)/

What kind of issues is it causing now? Just what's described in issue 220? Why would a random target in the middle of the graph would be deleted from the cache and others not? Is this a realistic issue, and what is its impact?

I don't object to removing the dirty logic if you confirm it's not introducing new issues or performance regressions.

dana-resonai commented 2 years ago

It causes issues also in https://github.com/resonai/ybt/pull/221 In the case of CppLib, we don't want to check the dirty bit since we might not have to build the target even though its dependency changed. So I have to separate the logic there.