[PATCH] D84931: [lit] Add --time-trace-output to lit

[Russell Gallop via Phabricator via llvm-commits]

russell.gallop marked 7 inline comments as done.
russell.gallop added inline comments.


================
Comment at: llvm/utils/lit/lit/reports.py:143
+        self.output_file = output_file
+        self.skipped_codes = {lit.Test.EXCLUDED,
+                              lit.Test.SKIPPED, lit.Test.UNSUPPORTED}
----------------
yln wrote:
> Unused
Now used.


================
Comment at: llvm/utils/lit/lit/reports.py:148
+    def write_results(self, tests, elapsed):
+        events =  [self._get_test_event(x) for x in tests]
+        first_timestamp = min(e['ts'] for e in events)
----------------
yln wrote:
> Please format the new code/use linter.
> Extra space after `events =`, missing space in `#Rebase`, etc.
Not sure what formatter is normally used for python in LLVM, applied autopep8 and fixed these.


================
Comment at: llvm/utils/lit/lit/reports.py:152
+        for e in events:
+            e['ts'] = e['ts'] - first_timestamp
+
----------------
yln wrote:
> Probably better to get the first timestamp in the beginning and compute the desired `ts` from the start rather than rebasing everything.
Done (if I have understood correctly).


================
Comment at: llvm/utils/lit/lit/reports.py:168
+            'ph': 'X',
+            'ts': int(start_time * 1000000.),
+            'dur': int(elapsed_time * 1000000.),
----------------
yln wrote:
> One quick additional question: I am assuming this is here because we want whole numbers?  I am not sure about Python numerics; there is no risk of overflowing here, right?
Good question. I had a look at this.

  - On python3 integers are of unlimited size so this is not a problem.
  - On python2 64bit an int is 64bit so this is not likely to be a problem
  - On python2 32bit an int is 32bit, which would be limit this to around 36 minutes. Python2 will promote this to a long if it goes over sys.maxint (even when creating an int with int()).

   Tested with the following on 32bit python2.7.15.
    i = int(2147483649.) # 2 more than sys.maxint as a float, like we are doing here.
    assert isinstance(i, long)

So it should be fine. Thanks for raising it.


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

https://reviews.llvm.org/D84931