The code was using C-style casts, accidentally stripping away const.
Change-Id: Ie34105a3974dcbf1879322cd15e2a9e392db95e8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/548476
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This improves performance with sandboxing.
One can have Bazel output performance data [1][2], which can be
viewed via chrome://tracing or https://ui.perfetto.dev/.
With Perfetto, the following SQL queries [3][4] can summarize
the sandboxing metrics, as well as the actual compilation
time. http://screen/5TxbeZTso4gNDfD
Note that the dur column is in nanoseconds, so we convert
to seconds. These numbers could further be divided by
the number of processes (in my case 48) to get a scaled output.
SELECT SUM(dur) / 1000000000.0 FROM slice WHERE name = "sandbox.createFileSystem";
SELECT SUM(dur) / 1000000000.0 FROM slice WHERE name = "sandbox.delete";
SELECT SUM(dur) / 1000000000.0 FROM slice WHERE name = "subprocess.run";
I benchmarked the local compilation of //:skia_public using
--config=clang_linux (our custom Linux toolchain). I was
sure to clear the Bazel cache before each run and not count
the time to download/update a toolchain for the first time.
The control measurements (without this CL) are:
Wall Time = 272.2s
sandbox.createFileSystem = 5466.9s
subprocess.run = 2961.0s
sandbox.delete = 4112.3s
With this CL:
Wall Time = 53.9s (5.05x faster)
sandbox.createFileSystem = 403.4s
subprocess.run = 1610.3s
sandbox.delete = 348.8s
The control measurement is a touch misleading. Due to it being
so slow, I had recommended developers use a ramdisk when building
on machines with sufficient RAM via the Bazel flag
--sandbox_base=/dev/shm
Here is the control measurement when using a RAM disk:
Wall Time = 21.2s
sandbox.createFileSystem = 58.9s
subprocess.run = 705.1s
sandbox.delete = 46.6s
With this CL and a RAM disk:
Wall Time = 19.2s (10% faster)
sandbox.createFileSystem = 21.8s
subprocess.run = 722.9s
sandbox.delete = 16.2s
For devs who cannot or are not using a RAM disk, this is
a huge win. With a RAM disk, it's a modest improvement.
On an RBE build, this had minimal impact. I did my best
to bust the action cache with a fake define and the before
and after times were about the same.
This was inspired by [5] and [6].
[1] Add --profile=/tmp/profile.gz to any command
[2] https://bazel.build/rules/performance#performance-profiling
[3] https://perfetto.dev/docs/quickstart/trace-analysis#sample-queries
[4] https://perfetto.dev/docs/analysis/sql-tables#slice
[5] 93f21c9ef3
[6] 311acff345
Change-Id: Iceb2606e86111159141a286d01fc002d09fe3fe4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547822
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
This is just the lowest level of the combination system. The higher
level iteration to accumulate the keys will be in a following CL.
Bug: skia:12701
Change-Id: I4015790a7bca49f11f8eb1a4fda32f235845c049
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547819
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This removes an if-check from the top of get_transition, removes an if-
check from the top of Lexer::next(), simplifies a bounds check, and
removes bitfields from the index array. Disappointingly, on my machine,
I can't measure any change at all; `get_transition` and `next`
stubbornly remain at about 4-5% of total nanobench time for
`sksl_large`. However, it's still simpler and hopefully slightly smaller
code.
Change-Id: If4187c01f350fe642b7af7cb6bd2c8250ca3c00e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/548396
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
codesize.skia.org was missing data and a misunderstanding of
file names was one cause.
To be consistent with Gold (and Perf?) we should use UTC as the
time zone for the folders and add hours. The server has been
switched to UTC in https://skia-review.googlesource.com/c/buildbot/+/548262/
This also fixes a race condition caused by upload the .tsv files
before the .json files. When the server sees a Pub/Sub event
for a .tsv file, it indexs the corresponding .json file.
Frequently, the .json file wouldn't have been uploaded yet,
so the indexing would fail.
Change-Id: Iabe64786db6e5c6020a3fc5dda244ccbe478c401
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/548357
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
The goal of this CL is to upload SKPs produced by the Test-Debian10-Clang-GCE-CPU-AVX2-x86_64-Debug-All-OldestSupportedSkpVersion into a new "old-skp" Gold corpus. This will make triaging SKPs easier.
Other approaches I considered:
- Define a new --oldSkp flag. Redundant since we can recover this information by looking at the --config flag.
- Define a new "old-skp" value accepted by the --src flag. More complex; requires larger changes in DM.cpp, and changes to the Test-Debian10-Clang-GCE-CPU-AVX2-x86_64-Debug-All-OldestSupportedSkpVersion task definition.
Bug: skia:13398
Change-Id: Ie8311e715eee0daf335e277132b7484a46b94489
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/548076
Commit-Queue: Leandro Lovisolo <lovisolo@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
In this CL, the GrSurfaceProxy's and GrDrawOpAtlas's label strings
are plumbed so that it can be stored in the label string of
GrGpuResource. onSetLabel method, which is called from setLabel
method of GrGpuResource, will pass labels to Skia OpenGL backend
using ANGLE's labeling API.
Bug: chromium:1164111
Change-Id: I516c06f0ebbf6bbe6d31ea5a4a64b2baeedd1560
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/545717
Commit-Queue: Greg Daniel <egdaniel@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
and rename it BeginBlock.
The motivation for this CL is that it is simpler for the combination
system and the shaders to iterate over their children themselves rather
than shoehorning it into AddToKey.
Bug: skia:12701
Change-Id: I334fbcb7ce83af0681fb06d8449125fdbe8b05f3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547446
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
We decided to be ok with an empty version because caching
shader compilations is not that much overhead compared to
the rest of the backends that just have to redo them.
Change-Id: I995b3c75a7e723a1188184f8753450634fa0b629
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/548356
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
We have a SkPaintParamsKeyBuilder at precompile time but not a
SkPipelineDataGatherer. We need the blend info at precompilation
time so that we can create a complete SkShaderCodeDictionary entry
for the precompiled combination(s).
Bug: skia:12701
Change-Id: I7686162ba93cb331f7c7dc2422e2d6937f367397
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547820
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
SKPBench tiles the SkPicture into multiple surfaces, with tile WH
differing between GPU and CPU backends. Graphite was being incorrectly
classified as a CPU backend and ended up using smaller tile sizes, so
the SkPicture would be played back many more times relative to Ganesh.
In addition, each surface's contents is a subset of the total picture,
so batching was artificially limited compared to Ganesh.
Added a call to Device::flushPendingWorkToRecorder() in
Surface_Graphite::onFlush(). This ensures DrawPass::Make() is called
when nanobench and viewer are measuring the bulk of the work
(viewer's "flush" time was always 0 for Graphite since the
DrawPass::Make was only being counted in the total time when it
was executed for swapBuffers()). Flushing in this manner also prevents
batching across loops in nanobench, or resetting/clearing prior loops
recorded draws when the benchmark starts with a fullscreen clear.
The SKPBench change should make all graphite benchmarks report lower
times compared to what's in perf.skia.org. The flush change should
increase their reported times for benchmarks that required multiple
loops to get an accurate time measurement (for expensive SKPs with
loops == 1, it shouldn't be affected).
Change-Id: I9256dbfc4c7c021377be8f5137b48036cc67e4a2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/548157
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
This unifies GrXferProcessor::BlendInfo and SkPipelineDataGatherer::BlendInfo.
The motivation for this is that I want to store a blend info on the
SkPaintParamsBuilder and having the BlendInfo in SkPipelineDataGatherer
made that difficult. Once moved to Blend.h it didn't make sense to have
the GrXferProcessor version.
Change-Id: I8c36feeef0bfe85dfc3f53b6bbb0193b29136bfd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547816
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
Instead of copying a vector, now we use a span. Also, migrate out a
parameter-equality test to a helper function (allowing a "match"
temporary variable to be removed, and improving readability a bit).
Change-Id: Ic7f3274f244f8dd9444a6780c08465f9026b63f4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/548156
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change-Id: Ifc4616c99bddd7d40d5a0bd2dd3c57bfb973500f
Bug: skia:12559
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547825
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
When an array type like `float[4]` is parsed, we now search for and
attempt to reuse an existing Type object instead of creating a new array
Type each time. These redundant array Types were stashed in the symbol
table, so this even rippled out to wasted space in our dehydrated data.
For builtin types, we stash their array types in the topmost non-builtin
symbol table of the program. For non-builtin types (structs), we play
it safe by stashing the array type in the current symbol table to avoid
mixing up types that share a name. Today's SkSL only allows structs at
the top-level and doesn't actually have a way to create two Types with
the same name, so this is perhaps an overabundance of caution.
Change-Id: I6feed44b7e15be57f0cac93b07b0d43dffb768b8
Bug: skia:12329
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547826
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This CL also removes a few from SkSLUtil but the majority of these will
be cleaned up in a followup. (Some of these are currently in active use
in SkSL.)
Change-Id: I7a018d3f6d8d21d69805f91d81a49c09636e4661
Bug: skia:12559
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547818
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Similar to other animatable text props, implement support for stroke
width.
Change-Id: Ia6ef65e8528edfec20def1b77da8df817e56cc7e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/546244
Reviewed-by: Jorge Betancourt <jmbetancourt@google.com>
Commit-Queue: Florin Malita <fmalita@google.com>
We have a TODO to get rid of the accessor functions. We can use direct
member pointers instead.
Change-Id: I81470d8e03905be583677ef41a6de3547c8b318e
Bug: skia:12559
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547823
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This caps-bit was always set to true. The last reference was removed
here: http://review.skia.org/512936
Change-Id: Ifbad66f58847f7bd8579c837de3d3b5428625e34
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547821
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Currently, the position and source rectangle are stored for each
glyph. The atlas entry has the needed information to reconstruct
the rectangle. Switch to storing the LeftTop of each glyph,
and use the rect from the atlas entry to get the width and height.
Adjust all the serialize/deserialize code to use the new format.
Change-Id: Ifb8524fb3a53d2f52059707918c732804e7f093a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547285
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Herb Derby <herb@google.com>
It fails on the Nvidia Linux GLSL drivers used in the Golo.
Change-Id: I79cfbb9955aef6a6a57d22c82ab94a657434be7d
Bug: skia:13395
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547447
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
This doesn't work on Nvidia's desktop Linux drivers, probably due to the
same root cause as skia:13035. (Reported the bug to Khronos at
https://gitlab.khronos.org/Tracker/vk-gl-cts/-/issues/3748)
Change-Id: Iafe73df72e175f5ccb6f4663e5b102fe7efaf57c
Bug: skia:13395
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547443
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
We had all the moving parts in place, but had neglected to check
`simplify_constant_equality` for structs. It worked for structs that
boiled down to the same expression tree, but not for mismatched structs.
Comparison of known structs should be allowed in a constant-expression.
Change-Id: I3a52abf4530d1fe25d17eb482c1e29b5b8e03cf6
Bug: skia:13387
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547444
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This is necessary for proper constant-folding, and can help us generate
better code. Most examples are pretty contrived, but a realistic benefit
can be found in inlined code (see TrivialArgumentsInlineDirectly.glsl).
Change-Id: I86aac9d48a70ae35a15109fd0b04400ae39a3b47
Bug: skia:13387
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547277
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This reverts commit 3ba37e1d38.
Reason for revert: disabled test on Win+Intel/Android+Adreno6xx GPUs
(skia:13393)
Broke out non-folding tests to a separate test due to failures on
Quadro/RTX3060. (skia:13395)
Original change's description:
> Revert "Add unit test covering struct field folding."
>
> This reverts commit 0cbba91940.
>
> Reason for revert: returns red on Adreno and Intel
>
> Original change's description:
> > Add unit test covering struct field folding.
> >
> > At present, we don't try to optimize away `myStruct.myField` accesses
> > even when `myStruct` is known or constant, so the output from the test
> > is not too impressive.
> >
> > Change-Id: I563559e5cdc6c2669d69ec78ad8ca09d3be02a68
> > Bug: skia:13387
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547276
> > Auto-Submit: John Stiles <johnstiles@google.com>
> > Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> > Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> > Commit-Queue: John Stiles <johnstiles@google.com>
>
> Bug: skia:13387
> Change-Id: I2e651ddb82fac08cdc16fa8b77696cdd314e805f
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547438
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Change-Id: Ic86ff6b1034363d1343793f94e3ba707adb2fcc3
Bug: skia:13387, skia:13393, skia:13395
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547439
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
These values are always bool, so we can eliminate several levels of
indirection.
Change-Id: I074e9bd7a8f984776f4c710f640d761dee4258c2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/546862
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Bug: skia:12703
Change-Id: I6949fb2de3213c770756a3be21c1d47387a40e10
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/545238
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
This reverts commit 0cbba91940.
Reason for revert: returns red on Adreno and Intel
Original change's description:
> Add unit test covering struct field folding.
>
> At present, we don't try to optimize away `myStruct.myField` accesses
> even when `myStruct` is known or constant, so the output from the test
> is not too impressive.
>
> Change-Id: I563559e5cdc6c2669d69ec78ad8ca09d3be02a68
> Bug: skia:13387
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547276
> Auto-Submit: John Stiles <johnstiles@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
Bug: skia:13387
Change-Id: I2e651ddb82fac08cdc16fa8b77696cdd314e805f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547438
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Change-Id: I6dab23fbd4d663e4031e3c6f568471f266f26936
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/547016
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
