3 ms·
I think the author undersold the investigation they did. They understood the problem rather convincingly. But the blog post as written is misguided: > I never
by millstone 6y ago
I think the author undersold the investigation they did. They understood the problem rather convincingly. But the blog post as written is misguided:
> I never investigated why the test had suddenly started failing but I suspect that the timer frequency or the timer start point had changed,
Do not act on suspicion alone.
Sometimes sporadic test failures are problems in the test framework. Suspicion + evidence becomes a hypothesis. Find a way to reproduce the failure, that leads you to understand why the test usually passes, then you can fix it with confidence.
Other times sporadic test failures point to problems in deeper layers. Maybe some errant signal handler was borking the FP rounding mode. Maybe there's real hardware errata. We may never know.
> It bothered me that it required esoteric floating-point knowledge to understand this problem so I wanted to fix googletest
This is a different problem. The test is failing, and also Google Test is written in a way that requires esoteric FP knowledge. Fix the first, then refactor to fix the second. These should absolutely be two independent commits. I think this opportunity was missed.
> I think that the googletest fix is ultimately more important than fixing the Chromium test
This is a false dichotomy. You can fix both!
If you increase the error bounds for some test, the failure no longer repros. That may or may not be the correct fix, but you have to be able to justify it.
In this case, one could mock the timer and search for times that would repro the failure. A nice FP trick is avoiding i386: its 80-bit fpu masks lots of other problems.
- muststopmyths 6y ago>These should absolutely be two independent commits. I think this opportunity was missed. look like two to me unless I'm missing something ? https://chromium.googlesource.com/chromium/src/+/6c2427457b0c5ebaefa5c1a6003117ca8126e7bc https://chromium.googlesource.com/chromium/src/+/6c2427457b0... https://github.com/google/googletest/commit/b5687db554a295e697f5d459cf6d3f343d2ca179#diff-9284ae098945ddfc48ad41d72fd5e687 https://github.com/google/googletest/commit/b5687db554a295e6...
- brucedawson 6y ago> These should absolutely be two independent commits. > I think this opportunity was missed. Uh, they were two independent commits. The test in Chromium was fixed in 2017. The change to googletest landed a few weeks ago in 2020. Separated by three years and in two different repositories - that sounds independent to me. > This is a false dichotomy. You can fix both! Yep, did that. FWIW the failure was originally reported on an x64 test, so the i386 FPU with its 80-bit registers was not used.
- brucedawson 6y agoBTW, I double checked and I'm reasonably certain that the test failure happened because we started running the test on machines with a lower QueryPerformanceCounter frequency (2.148 MHz) and the simulated counter values then created larger times that necessarily had less precision. I updated the post to include that. Note that the error bounds are only increased for those test values that are unrealistically large. The error bounds are as small as possible for the more "normal" test values.