Bug 103943 - nrwt: derive per-thread, per-worker stats from result summaries
Summary: nrwt: derive per-thread, per-worker stats from result summaries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 103824 103951
  Show dependency treegraph
 
Reported: 2012-12-03 16:13 PST by Dirk Pranke
Modified: 2012-12-04 14:51 PST (History)
4 users (show)

See Also:


Attachments
Patch (25.02 KB, patch)
2012-12-03 16:20 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
collect both test_run_time and total_run_time (25.71 KB, patch)
2012-12-04 11:57 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix typos, lint (26.20 KB, patch)
2012-12-04 13:26 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix typos, lint, verify manually (26.22 KB, patch)
2012-12-04 13:29 PST, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-12-03 16:13:08 PST
nrwt: derive per-thread, per-worker stats from result summaries
Comment 1 Dirk Pranke 2012-12-03 16:20:49 PST
Created attachment 177367 [details]
Patch
Comment 2 Ojan Vafai 2012-12-04 10:47:34 PST
Comment on attachment 177367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177367&action=review

> Tools/ChangeLog:21
> +        Also, this changes the stats so that we report only the time taken
> +        to actually run the test, rather than the time taken to run the test
> +        plus run matching reftests, compute diffs, etc. It's unclear which
> +        number is actually more interesting, but I suspect most people
> +        expect the stats to report the former; we can always add the latter
> +        back in if needed.

I think for the individual test timings we care about the time actually spent in DRT. But for the aggregate numbers we care about the whole time. For example, all your numbers for the time spent in workers or per-directory will now not include time spent doing the other work, no? That seems less than ideal.
Comment 3 Dirk Pranke 2012-12-04 11:09:49 PST
(In reply to comment #2)
> (From update of attachment 177367 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177367&action=review
> 
> > Tools/ChangeLog:21
> > +        Also, this changes the stats so that we report only the time taken
> > +        to actually run the test, rather than the time taken to run the test
> > +        plus run matching reftests, compute diffs, etc. It's unclear which
> > +        number is actually more interesting, but I suspect most people
> > +        expect the stats to report the former; we can always add the latter
> > +        back in if needed.
> 
> I think for the individual test timings we care about the time actually spent in DRT. But for the aggregate numbers we care about the whole time. For example, all your numbers for the time spent in workers or per-directory will now not include time spent doing the other work, no? That seems less than ideal.

Yeah, you're right. I was being a little lazy. I will collect both numbers.
Comment 4 Dirk Pranke 2012-12-04 11:57:09 PST
Created attachment 177522 [details]
collect both test_run_time and total_run_time
Comment 5 Ojan Vafai 2012-12-04 12:24:55 PST
Comment on attachment 177522 [details]
collect both test_run_time and total_run_time

View in context: https://bugs.webkit.org/attachment.cgi?id=177522&action=review

> Tools/Scripts/webkitpy/layout_tests/models/test_results.py:54
> +        self.total_run_time = 0  # The time taken to run the test plus any references, compute diffs, etc.

Where do we set this?
Comment 6 Dirk Pranke 2012-12-04 12:26:38 PST
(In reply to comment #5)
> (From update of attachment 177522 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177522&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_results.py:54
> > +        self.total_run_time = 0  # The time taken to run the test plus any references, compute diffs, etc.
> 
> Where do we set this?


Here, next to where we set the worker name and shard name:

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:304
> +        result.total_test_time = time.time() - start
Comment 7 Ojan Vafai 2012-12-04 13:14:34 PST
total_test_time != total_run_time
Comment 8 Dirk Pranke 2012-12-04 13:26:02 PST
Created attachment 177535 [details]
fix typos, lint
Comment 9 Dirk Pranke 2012-12-04 13:29:41 PST
Created attachment 177537 [details]
fix typos, lint, verify manually
Comment 10 Dirk Pranke 2012-12-04 13:30:24 PST
(In reply to comment #7)
> total_test_time != total_run_time

Whoops. Fixed now. Clearly I need some tests for this and don't have them, but if it's okay with you I'd like to address that in a separate patch. I verified things look right manually.
Comment 11 Dirk Pranke 2012-12-04 14:51:13 PST
Committed r136579: <http://trac.webkit.org/changeset/136579>