[PATCH] D66411: Fix -ftime-trace breaking flame-graph assumptions

[Russell Gallop via Phabricator via llvm-commits]

russell.gallop marked an inline comment as done.
russell.gallop added inline comments.


================
Comment at: llvm/unittests/Support/TimeProfilerTest.cpp:19
+// Run enough iterations to reliably see this issue in Release.
+#define ITERATIONS 100000
+#else
----------------
rnk wrote:
> russell.gallop wrote:
> > rnk wrote:
> > > I'm not sure this test has that much value because it will not fail deterministically if you revert the fix to bring back the truncation bugs.
> > Thanks for the review. I agree it's not ideal, this was the best way I could think of for testing this behaviour.
> > 
> > It might be possible to set the time values to appropriate values to trigger this but that would require exposing a fair bit more of TimeProfiler.cpp.
> > 
> > One alternative might be to have a check in Write() that reconstructed the flame graph and asserted if there was a problem. It would require a very large file (or a lot of files) to reliably hit the issue though.
> > 
> > Or is this okay to send without a test?
> Test aside, adding the assert seems valuable. As it is written now, it should hold unless the steady clock lies, right?
> 
> For the test, how long does the test normally take? If it takes more than 8s-10s, I think I would prefer to remove it and commit this without a test.
> Test aside, adding the assert seems valuable. As it is written now, it should hold unless the steady clock lies, right?

Yes, I think so. I'll try to add that.

> For the test, how long does the test normally take? If it takes more than 8s-10s, I think I would prefer to remove it and commit this without a test.

I measure 5-7 seconds in Release which is quite a large increase to SupportTests.exe (previously 10s).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66411/new/

https://reviews.llvm.org/D66411