Issue 10919 in skia: compile_sksl_tests: improve performance on Windows

9 views
Skip to first unread message

johns… via monorail

unread,
Nov 9, 2020, 4:37:08 PM11/9/20
to bu...@skia.org
Status: Accepted
Owner: johns...@google.com
Area: SkSL
Priority: Low
Type: Defect

New issue 10919 by johns...@google.com: compile_sksl_tests: improve performance on Windows
https://bugs.chromium.org/p/skia/issues/detail?id=10919

Currently, compile_sksl_tests spawns a separate process for `skslc` for each golden output that needs to be created. This is fast on Linux/Mac but process-spawning overhead on Windows is much higher.

It would probably be a significant improvement on Windows (and at least slightly better on all platforms) to spawn a single skslc which can compile multiple golden outputs in a row.

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

bugdroid via monorail

unread,
Nov 12, 2020, 10:08:47 AM11/12/20
to bu...@skia.org

Comment #1 on issue 10919 by bugdroid: compile_sksl_tests: improve performance on Windows
https://bugs.chromium.org/p/skia/issues/detail?id=10919#c1

The following revision refers to this bug:
https://skia.googlesource.com/skia/+/a1e8fe3a2b9415264bfec00026a52d01aaac9e35

commit a1e8fe3a2b9415264bfec00026a52d01aaac9e35
Author: John Stiles <johns...@google.com>
Date: Thu Nov 12 15:05:27 2020

Update skslc to compile multiple files during one invocation.

skslc can now take a `.worklist` file as an input, containing multiple
"command lines" to run in sequence. compile_sksl_tests.py now assembles
a worklist file and runs skslc one time, rather than running skslc
once per each target. This improves compile times on Windows
significantly (where spawning skslc hundreds of times is much more
expensive than on Linux/Mac).

One subtle behavioral difference with .worklist files: if an error is
encountered, it is written to the output file instead of to stdout.
Previously, compile_sksl_tests was in charge of for capturing stdout
and overwriting the compiler output with the error message, but this
doesn't work when many files are being compiled (which errors are
associated with which files?)

This refactor exposed a minor latent bug--when encountering an error,
skslc would previously exit() immediately without closing its
FileOutputStream. This led to an assertion when exit() was replaced with
normal returns. Since FileOutputStream is only used by skslc, and in
every case the desired behavior is just to close the stream cleanly,
FileOutputStream now closes the file in its destructor instead of
asserting that we haven't done so.

Change-Id: Ia55baff0c11fe466923bde2e0c944df9f2ccd092
Bug: skia:10919
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334099
Reviewed-by: Brian Osman <brian...@google.com>
Commit-Queue: John Stiles <johns...@google.com>
Auto-Submit: John Stiles <johns...@google.com>

[modify] https://crrev.com/a1e8fe3a2b9415264bfec00026a52d01aaac9e35/gn/compile_sksl_tests.py
[modify] https://crrev.com/a1e8fe3a2b9415264bfec00026a52d01aaac9e35/src/sksl/SkSLMain.cpp
[modify] https://crrev.com/a1e8fe3a2b9415264bfec00026a52d01aaac9e35/src/sksl/SkSLFileOutputStream.h

johns… via monorail

unread,
Nov 12, 2020, 11:17:56 AM11/12/20
to bu...@skia.org
Updates:
Status: Fixed

Comment #2 on issue 10919 by johns...@google.com: compile_sksl_tests: improve performance on Windows
https://bugs.chromium.org/p/skia/issues/detail?id=10919#c2

(No comment was entered for this change.)

bugdroid via monorail

unread,
Nov 12, 2020, 12:20:26 PM11/12/20
to bu...@skia.org

Comment #3 on issue 10919 by bugdroid: compile_sksl_tests: improve performance on Windows
https://bugs.chromium.org/p/skia/issues/detail?id=10919#c3


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/ba06e1ae5da6c80887fe6a1211a054f481188120

commit ba06e1ae5da6c80887fe6a1211a054f481188120
Author: John Stiles <johns...@google.com>
Date: Thu Nov 12 17:20:09 2020

Improve performance of skia_compile_processors.

Previously, we would instantiate skslc twice, and clang-format once, for
each fragment processor. We now batch all the work into a single
invocation of each tool--skslc is called once with a worklist file, and
clang-format is asked to clean all the generated files at once. This
will improve build times very substantially on Windows, and should
provide a small benefit on Mac/Linux as well.

Change-Id: I97ac1f22bf19298dfac1c02e1a28a106cfc8491d
Bug: skia:10919
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334420
Commit-Queue: John Stiles <johns...@google.com>
Commit-Queue: Brian Osman <brian...@google.com>
Auto-Submit: John Stiles <johns...@google.com>
Reviewed-by: Brian Osman <brian...@google.com>

[modify] https://crrev.com/ba06e1ae5da6c80887fe6a1211a054f481188120/gn/compile_processors.py

bugdroid via monorail

unread,
Nov 13, 2020, 4:28:33 PM11/13/20
to bu...@skia.org

Comment #4 on issue 10919 by bugdroid: compile_sksl_tests: improve performance on Windows
https://bugs.chromium.org/p/skia/issues/detail?id=10919#c4


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/3e1b771ce465a25e50aabcdd2212edc82b820c3d

commit 3e1b771ce465a25e50aabcdd2212edc82b820c3d
Author: John Stiles <johns...@google.com>
Date: Fri Nov 13 21:27:20 2020

Replace skslc worklist files with -- delimited command lines.

Command lines with delimiters are a simpler approach; they don't require
a scratch file to be created and parsed. (I didn't consider this
approach until after implementing worklists.)

This also fixes a minor issue with result codes when processing multiple
files at once; in particular, unit tests can ignore compile errors, but
regular fragment processor compilation should treat compile errors as
fatal and stop the build.

Change-Id: I3f153e7670d757c6b021bf60a260a2cd3f2090aa
Bug: skia:10919
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334428
Reviewed-by: Ethan Nicholas <ethann...@google.com>
Commit-Queue: Ethan Nicholas <ethann...@google.com>
Auto-Submit: John Stiles <johns...@google.com>

[modify] https://crrev.com/3e1b771ce465a25e50aabcdd2212edc82b820c3d/gn/compile_sksl_tests.py
[modify] https://crrev.com/3e1b771ce465a25e50aabcdd2212edc82b820c3d/src/sksl/SkSLMain.cpp
[modify] https://crrev.com/3e1b771ce465a25e50aabcdd2212edc82b820c3d/gn/compile_processors.py

bugdroid via monorail

unread,
Nov 18, 2020, 1:03:21 PM11/18/20
to bu...@skia.org

Comment #5 on issue 10919 by bugdroid: compile_sksl_tests: improve performance on Windows
https://bugs.chromium.org/p/skia/issues/detail?id=10919#c5


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/7b239054d99d57768e54985712294b69e64e3042

commit 7b239054d99d57768e54985712294b69e64e3042
Author: Brian Osman <brian...@google.com>
Date: Wed Nov 18 18:02:25 2020

Revert "Replace skslc worklist files with -- delimited command lines."

This reverts commit 3e1b771ce465a25e50aabcdd2212edc82b820c3d.

Reason for revert: Not working on Windows.

Original change's description:

> Replace skslc worklist files with -- delimited command lines.
>
> Command lines with delimiters are a simpler approach; they don't require
> a scratch file to be created and parsed. (I didn't consider this
> approach until after implementing worklists.)
>
> This also fixes a minor issue with result codes when processing multiple
> files at once; in particular, unit tests can ignore compile errors, but
> regular fragment processor compilation should treat compile errors as
> fatal and stop the build.
>
> Change-Id: I3f153e7670d757c6b021bf60a260a2cd3f2090aa
> Bug: skia:10919
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334428
> Reviewed-by: Ethan Nicholas <ethann...@google.com>
> Commit-Queue: Ethan Nicholas <ethann...@google.com>
> Auto-Submit: John Stiles <johns...@google.com>

Reply all
Reply to author
Forward
0 new messages