New issue
Advanced search Search tips

Issue 821584 link

Starred by 2 users
Status: Fixed
Owner:
khushals...@chromium.org
Closed: Jul 11
Cc:
vmp...@chromium.org
enne@chromium.org
khushals...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocking:
issue 757605



Sign in to add a comment

OOP serialization improvements.

Project Member Reported by khushals...@chromium.org, Mar 13 2018 
Profiling thread times on some key silk cases, here are some of the hot code paths that came up during serialization.

SkTextBlob serialization: An additional copy and allocation in skia (https://chromium-review.googlesource.com/c/chromium/src/+/961274). Trying to get a fix in skia.
Another thing that came up is a temp SkData allocation done to serialize the typeface id in skia (https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkTextBlob.cpp?l=903). It can probably be avoided within skia, since its all internal.

SkTextBlob deserialization: This was an unnecessary copy for creating a transient SkData (https://chromium-review.googlesource.com/c/chromium/src/+/959439).

The text blob serialization stuff applies to other skia types we serialize in the same way (SkFlattenables).
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 14 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/883ad990be2af1bc0e2111906d1d16b9cb7c6280

commit 883ad990be2af1bc0e2111906d1d16b9cb7c6280
Author: Khushal <khushalsagar@chromium.org>
Date: Wed Mar 14 07:31:16 2018

cc: Avoid SkData malloc and copy for SkTextBlobs.

PaintOpReader::Read for SkData is used by 2 code-paths, deserializing
SkTextBlobs and AnnotateOps. While AnnotateOp needs to copy the SkData,
the copy for SkTextBlobs is completely unnecessary.

This also uncovered skia caching keyed on text blob ids in
GrAtlasTextContext, which we'll miss in OOP by re-serializing the text
blobs each time. This should be addressed by caching these on the
service side in a follow up.

R=enne@chromium.org
BUG:  821584 

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I77b47621e0f0bca225377968b02ca5f401804bc0
Reviewed-on: https://chromium-review.googlesource.com/959439
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543025}
[modify] https://crrev.com/883ad990be2af1bc0e2111906d1d16b9cb7c6280/cc/paint/paint_op_reader.cc
[modify] https://crrev.com/883ad990be2af1bc0e2111906d1d16b9cb7c6280/cc/paint/paint_op_writer.cc
[modify] https://crrev.com/883ad990be2af1bc0e2111906d1d16b9cb7c6280/cc/paint/paint_text_blob.h

Comment 2 by khushals...@chromium.org, Apr 5 2018 

Also left is re-using same text blobs on the service to hit skia caches in GrAtlasTextContext.

Comment 3 by khushals...@chromium.org, Apr 5 2018

Cc: khushals...@chromium.org vmp...@chromium.org
 Issue 785434  has been merged into this issue.

Comment 4 by enne@chromium.org, Apr 5 2018

Blocking: 757605
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 5 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c96940cb27bc403ba1d3806b26e7160b3d47133d

commit c96940cb27bc403ba1d3806b26e7160b3d47133d
Author: Khushal <khushalsagar@chromium.org>
Date: Thu Apr 05 22:54:47 2018

cc: Avoid extra allocation and copy for SkTextBlob serialization.

When serializing a SkTextBlob, skia flattens it into allocated memory
which is then copied into PaintOpWriter's memory. Change this to
directly serialize into the writer's memory.

R=enne@chromium.org, reed@google.com
BUG:821584

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I644a02aba1b8f5b204596eaf2b04d8763a9cce37
Reviewed-on: https://chromium-review.googlesource.com/961274
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548599}
[modify] https://crrev.com/c96940cb27bc403ba1d3806b26e7160b3d47133d/cc/paint/paint_op_buffer_unittest.cc
[modify] https://crrev.com/c96940cb27bc403ba1d3806b26e7160b3d47133d/cc/paint/paint_op_reader.cc
[modify] https://crrev.com/c96940cb27bc403ba1d3806b26e7160b3d47133d/cc/paint/paint_op_writer.cc
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Jul 11 

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/10114f24a40000

Buildbucket says the build completed successfully, but Pinpoint can't find the isolate hash.
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Jul 11 

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/11e98dfaa40000

Comment 10 by khushals...@chromium.org, Jul 11

Status: Fixed (was: Started)
I tried a hack for inifinitely caching all text blobs on the GPU to see what the impact of potentially using the transfer cache for these would be, and looking at cpu usage its a wash. I don't think its worth doing, unless we see a case come up where it has an impact.
►

Sign in to add a comment