You cannot select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
5e1e4d710b
Spotted by our friend "go test -race": WARNING: DATA RACE Write at 0x0000010522d8 by goroutine 69: mvdan.cc/garble.readFile() garble/main_test.go:124 +0x23a mvdan.cc/garble.binsubstr() garble/main_test.go:141 +0xc4 github.com/rogpeppe/go-internal/testscript.(*TestScript).run() github.com/rogpeppe/go-internal@v1.8.1-0.20211023094830-115ce09fd6b4/testscript/testscript.go:496 +0x9e8 [...] Previous write at 0x0000010522d8 by goroutine 60: mvdan.cc/garble.readFile() garble/main_test.go:124 +0x23a mvdan.cc/garble.binsubstr() garble/main_test.go:141 +0xc4 github.com/rogpeppe/go-internal/testscript.(*TestScript).run() github.com/rogpeppe/go-internal@v1.8.1-0.20211023094830-115ce09fd6b4/testscript/testscript.go:496 +0x9e8 [...] This wasn't a data race that we spotted via failures in practice, as it only affected test code since July. The race is due to the fact that each test script runs as a parallel sub-test within the same Go program, sharing all globals. As such, a single "cached binary" global is read and written with races. Moreover, note that the caching always missed. I briefly rewrote the code to avoid the race via a sync.Map keyed by absolute filenames, and while that removed the data race, the caching never actually hit. To have a cache hit, we need an absolute path to already be in the cache and for it to not have been modified since it was last cached. That is: modify-bin-1 foo binsubstr foo 'abc' # miss binsubstr foo 'def' # hit; use the cached "/tmp/[...]/foo" entry modify-bin-2 foo binsubstr foo 'abc' # miss However, the test scripts don't do contiguous binsubstr calls like these. Instead, they join repeated binsubstr calls: modify-bin-1 foo binsubstr foo 'abc' 'def' # miss modify-bin-2 foo binsubstr foo 'abc' # miss For that reason, remove the extra code entirely. I didn't notice any change to the performance of "go test -short" with a warm build cache, with: go test -c ./garble.test -test.short #warm cache benchcmd -n 5 TestShort ./garble.test -test.short name old time/op new time/op delta TestShort 4.62s ±12% 4.35s ±12% ~ (p=0.310 n=5+5) name old user-time/op new user-time/op delta TestShort 16.8s ± 3% 16.7s ± 3% ~ (p=0.690 n=5+5) name old sys-time/op new sys-time/op delta TestShort 7.28s ± 1% 7.26s ± 2% ~ (p=0.841 n=5+5) name old peak-RSS-bytes new peak-RSS-bytes delta TestShort 305MB ± 0% 306MB ± 0% ~ (p=0.421 n=5+5) Finally, start using "go test -race" on Linux on CI, which should have made the PR back in July red before merging. |
3 years ago | |
|---|---|---|
| .. | ||
| workflows | 3 years ago | |
| FUNDING.yml | 5 years ago | |
| ISSUE_TEMPLATE.md | 4 years ago | |