Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1070943002:
[SkTextBlob] Custom run font record (Closed)
Description[SkTextBlob] Custom run font record
Instead of using a full-blown SkPaint to store run font info, use a
custom structure.
This saves 96 bytes / run on 64bit platforms.
R=reed@google.com,mtklein@google.com,joshualitt@google.com
Committed: https://skia.googlesource.com/skia/+/055f6b59d879b2adac52748ea5a58c8a05bf501c
Patch Set 1 #
Total comments: 4
Patch Set 2 : review comments #
Total comments: 3
Patch Set 3 : drop SkNoncopyable #Patch Set 4 : SkNoncopyable + EBCO workaround #
Total comments: 6
Patch Set 5 : review comments #Messages
Total messages: 25 (5 generated)
We'll switch to SkFont eventually, but that'll take some SkFont API tweaks. In the meantime, we can use a local type.
On 2015/04/08 21:06:53, f(malita) wrote: > We'll switch to SkFont eventually, but that'll take some SkFont API tweaks. In > the meantime, we can use a local type. Did I pick the wrong trybots? All that redness is unrelated...
On 2015/04/08 21:17:33, f(malita) wrote: > On 2015/04/08 21:06:53, f(malita) wrote: > > We'll switch to SkFont eventually, but that'll take some SkFont API tweaks. In > > the meantime, we can use a local type. > > Did I pick the wrong trybots? All that redness is unrelated... Those bots are red right now. Good bots in theory.
https://codereview.chromium.org/1070943002/diff/1/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/1070943002/diff/1/src/core/SkTextBlob.cpp#new... src/core/SkTextBlob.cpp:17: class RunFont { better : SkNoncopyable this. https://codereview.chromium.org/1070943002/diff/1/src/core/SkTextBlob.cpp#new... src/core/SkTextBlob.cpp:71: uint32_t fFlags; Do we have two free bits in fFlags to store the SkPaint::Hinting? That'd pack this down into 24 bytes.
https://codereview.chromium.org/1070943002/diff/1/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/1070943002/diff/1/src/core/SkTextBlob.cpp#new... src/core/SkTextBlob.cpp:17: class RunFont { On 2015/04/08 21:42:18, mtklein wrote: > better : SkNoncopyable this. Aww, but that adds 8 bytes to the size (wiping out the packing gains): With SkNoncopyable inheritance sizeof(RunRecord): 56, sizeof(BlobFont): 32 With no base class: sizeof(RunRecord): 48, sizeof(BlobFont): 24 :( https://codereview.chromium.org/1070943002/diff/1/src/core/SkTextBlob.cpp#new... src/core/SkTextBlob.cpp:71: uint32_t fFlags; On 2015/04/08 21:42:18, mtklein wrote: > Do we have two free bits in fFlags to store the SkPaint::Hinting? That'd pack > this down into 24 bytes. Yup, looks like SkPaint's flags fit in 16 bits just fine. Done.
https://codereview.chromium.org/1070943002/diff/20001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/1070943002/diff/20001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:17: class RunFont : SkNoncopyable { Woah, if this adds size, let's not do that. Let's just hide the copy and assignment constructors manually. I've always assumed empty base class optimization would apply for SkNoncopyable. It's bizarre that it adds size.
https://codereview.chromium.org/1070943002/diff/20001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/1070943002/diff/20001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:17: class RunFont : SkNoncopyable { On 2015/04/09 09:44:19, mtklein wrote: > Woah, if this adds size, let's not do that. Let's just hide the copy and > assignment constructors manually. Done. > I've always assumed empty base class optimization would apply for SkNoncopyable. > It's bizarre that it adds size. Yeah, I'd like to understand what's going on. This doesn't make sense to me either.
lgtm
https://codereview.chromium.org/1070943002/diff/20001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/1070943002/diff/20001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:17: class RunFont : SkNoncopyable { On 2015/04/09 13:07:09, f(malita) wrote: > > I've always assumed empty base class optimization would apply for > SkNoncopyable. > > It's bizarre that it adds size. > > Yeah, I'd like to understand what's going on. This doesn't make sense to me > either. OK, I see what's going on: this is triggered by the first member being an SkAutoTUnref, which itself is derived from SkNoncopyable => it inhibits EBCO. I can easily move fTypeface around here, but I wonder whether this affects other Skia classes.
On 2015/04/09 13:18:31, f(malita) wrote: > https://codereview.chromium.org/1070943002/diff/20001/src/core/SkTextBlob.cpp > File src/core/SkTextBlob.cpp (right): > > https://codereview.chromium.org/1070943002/diff/20001/src/core/SkTextBlob.cpp... > src/core/SkTextBlob.cpp:17: class RunFont : SkNoncopyable { > On 2015/04/09 13:07:09, f(malita) wrote: > > > I've always assumed empty base class optimization would apply for > > SkNoncopyable. > > > It's bizarre that it adds size. > > > > Yeah, I'd like to understand what's going on. This doesn't make sense to me > > either. > > OK, I see what's going on: this is triggered by the first member being an > SkAutoTUnref, which itself is derived from SkNoncopyable => it inhibits EBCO. > > I can easily move fTypeface around here, but I wonder whether this affects other > Skia classes. Tom and I were puzzling a while back about why a class with members like this: unsigned x,y; SkAutoTMalloc<...> z; was smaller than SkAutoTMalloc<...> z; unsigned x,y; Bet it's something similar. Seems like most of the time this doesn't matter, and it's nice to see SkNoncopyable up at the top. When it does matter, we've got plenty of only slightly less readable options to prevent copies.
On 2015/04/09 13:34:44, mtklein wrote: > Tom and I were puzzling a while back about why a class with members like this: > > unsigned x,y; SkAutoTMalloc<...> z; > was smaller than > SkAutoTMalloc<...> z; unsigned x,y; > > Bet it's something similar. Yup, sounds like the same issue. > Seems like most of the time this doesn't matter, and it's nice to see > SkNoncopyable up at the top. When it does matter, we've got plenty of only > slightly less readable options to prevent copies. Added SkNoncopyable inheritance back + EBCO workaround + size guard. Not sure whether this is more/less readable than the previous version - let me know what you think.
Either way seems fine to me, but I think I slightly prefer PS 3.
On 2015/04/09 14:01:47, mtklein wrote: > Either way seems fine to me, but I think I slightly prefer PS 3. Thanks.
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/1070943002/#ps60001 (title: "SkNoncopyable + EBCO workaround")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1070943002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 1070943002-60001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')
I sure like using a real class that just stores what we care about. https://codereview.chromium.org/1070943002/diff/60001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/1070943002/diff/60001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:70: // SkNoncopyable empty baseclass optimization. Can we link to a bug here, to describe this empty optimizations? I find this confusing. https://codereview.chromium.org/1070943002/diff/60001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:83: void* ptr; Can this be reordered to look like the field-order in RunFont? e.g. fSize, fScaleX; void* fSkewX flags Having all 3 scalars grouped is disconcerting since that is not how RunFont looks. https://codereview.chromium.org/1070943002/diff/60001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:182: RunFont fFont; Can/should this field be first, for alignment, since it contains a ptr? (i.e. before fCount)
https://codereview.chromium.org/1070943002/diff/60001/src/core/SkTextBlob.cpp File src/core/SkTextBlob.cpp (right): https://codereview.chromium.org/1070943002/diff/60001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:70: // SkNoncopyable empty baseclass optimization. On 2015/04/09 14:44:49, reed1 wrote: > Can we link to a bug here, to describe this empty optimizations? I find this > confusing. Done. https://codereview.chromium.org/1070943002/diff/60001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:83: void* ptr; On 2015/04/09 14:44:49, reed1 wrote: > Can this be reordered to look like the field-order in RunFont? > > e.g. > fSize, fScaleX; > void* > fSkewX > flags > > Having all 3 scalars grouped is disconcerting since that is not how RunFont > looks. Done. https://codereview.chromium.org/1070943002/diff/60001/src/core/SkTextBlob.cpp... src/core/SkTextBlob.cpp:182: RunFont fFont; On 2015/04/09 14:44:49, reed1 wrote: > Can/should this field be first, for alignment, since it contains a ptr? (i.e. > before fCount) Good catch, I think that shaves another 8 bytes off RunRecord! Done.
lgtm
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/1070943002/#ps80001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1070943002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/055f6b59d879b2adac52748ea5a58c8a05bf501c |
