Stop saving reftest.log when running locally

RESOLVED FIXED in Firefox 49

Status

Testing
Reftest
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
In bug 1271035 gps identified disk I/O as a major cause for reftest slowness. He was able to fix most of it, but the new highest source of I/O in reftest is reftest.log.

To note, we were only saving reftest.log when running via mach, so this won't impact automation in any way:
https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/mach_commands.py#204

I don't know why we are doing this given that the same output goes to stdout. And as dholbert pointed out, since bug 1034290 landed, that log contains raw structured logs, which are not at all useful for debugging. I propose we simply stop saving it to boost local reftest performance.

If anyone wishes to keep saving to a log, they can use:
./mach reftest --log-tbpl reftest.log

If they wish this to be the default behaviour they can make a machrc (or .machrc) in either topsrcdir, ~/.mozbuild or $MACHRC. Then add:
[alias]
reftest = reftest --log-tbpl reftest.log
(Assignee)

Comment 1

2 years ago
Created attachment 8750490 [details]
MozReview Request: Bug 1271448 - Stop saving reftest.log for |mach reftest| perf improvements, r?jgraham

In bug 1271035 gps identified disk I/O as a major cause for reftest slowness.
He was able to fix most of it, but the new highest source of I/O in reftest is
reftest.log.

To note, we were only saving reftest.log when running via mach, so this won't
impact automation in any way:
https://hg.mozilla.org/mozilla-central/file/043082cb7bd8/layout/tools/reftest/mach_commands.py#l204

I don't know why we are doing this given that the same output goes to stdout.
And as dholbert pointed out, since bug 1034290 landed, that log contains raw
structured logs, which are not at all useful for debugging. Given that it is
no longer useful and causes slowness, we should stop saving it.

If anyone wishes to keep saving to a log, they can use:
./mach reftest --log-tbpl reftest.log

If they wish this to be the default behaviour they can make a machrc (or
.machrc) in either topsrcdir, ~/.mozbuild or $MACHRC. Then add:
[alias]
reftest = reftest --log-tbpl reftest.log

Review commit: https://reviewboard.mozilla.org/r/51439/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51439/
Attachment #8750490 - Flags: review?(james)
(Assignee)

Comment 2

2 years ago
James, your name showed up in blame (though I suspect you refactored). Do you have any insight on whether this log was being used for something?

Comment 3

2 years ago
I'm almost certain dbaron will have an opinion here.

FTR, I like the approach of writing reftest.log from mach and not from JS. That should make bug 1271068 a WONTFIX (assuming we don't care about the performance of non-mach invocations).
Flags: needinfo?(dbaron)
> I don't know why we are doing this given that the same output goes to stdout.
> And as dholbert pointed out, since bug 1034290 landed, that log contains raw
> structured logs, which are not at all useful for debugging. Given that it is
> no longer useful and causes slowness, we should stop saving it.

Not sure who depends on this, or when it got added to the pipeline.  I'm fine with piping stdout to a file if that's faster.
Flags: needinfo?(dbaron)
(Assignee)

Comment 5

2 years ago
To clarify, we currently pipe output to both stdout and a file. Afaict, there is no good reason that we're piping to a file (on non-Android), especially since structured logging has made its contents a list of json objects (i.e hard to read).

And for those who want/need to pipe to a file, there's an easy workaround in comment 0.

(In reply to Gregory Szorc [:gps] from comment #3)
> FTR, I like the approach of writing reftest.log from mach and not from JS.
> That should make bug 1271068 a WONTFIX (assuming we don't care about the
> performance of non-mach invocations).

Automation does not go through mach, so this bug will only impact running locally. On automation, there is no reftest.log, though mozharness does write the normal raw log that is uploaded for all structured suites.
Comment on attachment 8750490 [details]
MozReview Request: Bug 1271448 - Stop saving reftest.log for |mach reftest| perf improvements, r?jgraham

https://reviewboard.mozilla.org/r/51439/#review48693

Sure. I don't think I ever knew why it did this in the first place and irrespective of any speed benefit, this brings reftests in line with other test harnesses.
Attachment #8750490 - Flags: review?(james) → review+

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af3d90a21e33

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af3d90a21e33
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1278577
You need to log in before you can comment on or make changes to this bug.