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

Don't mark sources as done as they might be dependencies elsewhere #2091

Merged
merged 6 commits into from Oct 4, 2021
Merged

Don't mark sources as done as they might be dependencies elsewhere #2091

merged 6 commits into from Oct 4, 2021

Conversation

tiagovtristao
Copy link
Contributor

@tiagovtristao tiagovtristao commented Sep 24, 2021

This PR fixes #2077.

  • Instead of marking sources as done (as they might be dependencies of other rules), do not include them in the list of dependencies that we iterate on. This slightly increases the time of preparing sources for targets where affected dependencies weren't previously being traversed/prepared.
  • Fixes a bug where although sources where marked as done previously, requires/provides functionality was still taking place over marked sources.
  • The logic is behind a FF as it might be disruptive.
  • Fixes benchmarks around IterInputs where the real metrics are one order of magnitude higher than those previously recorded.

Before this PR:

goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) E-2136 CPU @ 3.30GHz
BenchmarkIterInputsControl
BenchmarkIterInputsControl-12             520227              2155 ns/op             688 B/op          7 allocs/op
BenchmarkIterInputsSimple
BenchmarkIterInputsSimple-12              175366              7727 ns/op            1584 B/op          8 allocs/op
BenchmarkIterInputsNamedSources
BenchmarkIterInputsNamedSources-12        129492             11883 ns/op            4192 B/op         20 allocs/o

Benchmarks fixed:

goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) E-2136 CPU @ 3.30GHz
BenchmarkIterInputsControl
BenchmarkIterInputsControl-12             470923              2217 ns/op             688 B/op          7 allocs/op
BenchmarkIterInputsSimple
BenchmarkIterInputsSimple-12               10000            106134 ns/op           33837 B/op        415 allocs/op
BenchmarkIterInputsNamedSources
BenchmarkIterInputsNamedSources-12         10000            100276 ns/op           36443 B/op        427 allocs/o

Benchmarks fixed and IterInputs fixed:

goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) E-2136 CPU @ 3.30GHz
BenchmarkIterInputsControl
BenchmarkIterInputsControl-12             547437              2298 ns/op             688 B/op          7 allocs/op
BenchmarkIterInputsSimple
BenchmarkIterInputsSimple-12               13160             87224 ns/op           33835 B/op        415 allocs/op
BenchmarkIterInputsNamedSources
BenchmarkIterInputsNamedSources-12         10000            112870 ns/op           35142 B/op        421 allocs/op

Copy link
Member

@Tatskaari Tatskaari left a comment

Choose a reason for hiding this comment

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

Looks like this change will increase the number of files in build directories in a few places. We need to add output_is_complete in a few places e.g. the go import config rules.

Copy link
Member

@Tatskaari Tatskaari 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 to me :)

@tiagovtristao tiagovtristao merged commit 5447c07 into thought-machine:master Oct 4, 2021
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.

Dependency not available if marked as source on another one
2 participants