Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1465773003:
Pack and align SkStrokeRec to 4-byte boundary. (Closed)
|
Created:
5 years, 1 month ago by jvanverth1 Modified:
5 years, 1 month ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionPack and align SkStrokeRec to 4-byte boundary.
The new key for the distance field path cache will contain an
SkStrokeRec. This change guarantees that we don't have any hidden
padding that has garbage values, thereby preventing apparently
equal keys from hashing to two different values. This also has
the nice effect of reducing the size of SkStrokeRec from 24 bytes
to 16 bytes.
Committed: https://skia.googlesource.com/skia/+/897c9937636c2287bb217c76da9a56afb43050ac
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove anonymous struct #Patch Set 3 : Fix bool alignment #Patch Set 4 : Use u32; add comments #Patch Set 5 : One more comment #
Messages
Total messages: 25 (11 generated)
Description was changed from ========== Pack and align SkStrokeRec to 4-byte boundary. The new key for the distance field path cache will contain an SkStrokeRec. This change guarantees that we don't have any hidden padding that has garbage values, thereby preventing apparently equal keys from hashing to two different values. ========== to ========== Pack and align SkStrokeRec to 4-byte boundary. The new key for the distance field path cache will contain an SkStrokeRec. This change guarantees that we don't have any hidden padding that has garbage values, thereby preventing apparently equal keys from hashing to two different values. This also has the nice effect of reducing the size of SkStrokeRec from 24 bytes to 16 bytes. ==========
jvanverth@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com
lgtm
The CQ bit was checked by jvanverth@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1465773003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1465773003/1
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1465773003/diff/1/include/core/SkStrokeRec.h File include/core/SkStrokeRec.h (right): https://codereview.chromium.org/1465773003/diff/1/include/core/SkStrokeRec.h#... include/core/SkStrokeRec.h:118: struct { So, anonymous structs are not technically part of C++. (Anonymous unions are legal in C++ and C, and anonymous structs became legal in C11 but not C++11). Chrome will yell at you. :| It's a little silly because they're supported by all our compilers, and sometimes make for more readable code. On the other hand, I don't think you need this here. You should be fine to just put the uint32_t ... inline with the rest of the fields. They'll pack because they're adjacent. You might also consider: uint8_t fCap : 2; // SkPaint::Cap uint8_t fJoin : 2; // SkPaint::Join bool fStrokeAndFill : 1; uint32_t fUnusedBits : 27;
The CQ bit was unchecked by jvanverth@google.com
On 2015/11/20 14:57:25, mtklein wrote: > https://codereview.chromium.org/1465773003/diff/1/include/core/SkStrokeRec.h > File include/core/SkStrokeRec.h (right): > > https://codereview.chromium.org/1465773003/diff/1/include/core/SkStrokeRec.h#... > include/core/SkStrokeRec.h:118: struct { > So, anonymous structs are not technically part of C++. (Anonymous unions are > legal in C++ and C, and anonymous structs became legal in C11 but not C++11). > Chrome will yell at you. :| > > It's a little silly because they're supported by all our compilers, and > sometimes make for more readable code. > > On the other hand, I don't think you need this here. You should be fine to just > put the uint32_t ... inline with the rest of the fields. They'll pack because > they're adjacent. Done. > You might also consider: > uint8_t fCap : 2; // SkPaint::Cap > uint8_t fJoin : 2; // SkPaint::Join > bool fStrokeAndFill : 1; > uint32_t fUnusedBits : 27; I'd rather not have to worry about initializing fUnusedBits.
The CQ bit was checked by jvanverth@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1465773003/#ps40001 (title: "Fix bool alignment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1465773003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1465773003/40001
reed@google.com changed reviewers: + reed@google.com
Seems like there is still invisible padding, since we have an odd number of 16bit values, and we will get padded to a multiple of 4 bytes. If we want to create a new constraint on this struct that it be "hashable", I think that should be explicitly stated somewhere (comment?) where we declare the fields. e.g. // need to ensure we fully saturate our sizeof() so we can reliably hash this struct as an array... uint32_t fCap : 2; uint32_t fJoin : 2; uint32_t fOther : 1; uint32_t fUnused : 32 - 5;
The CQ bit was unchecked by reed@google.com
On 2015/11/20 at 15:27:17, reed wrote: > Seems like there is still invisible padding, since we have an odd number of 16bit values, and we will get padded to a multiple of 4 bytes. Nope, this should pack as 4 bytes as written. > If we want to create a new constraint on this struct that it be "hashable", I think that should be explicitly stated somewhere (comment?) where we declare the fields. > > e.g. > // need to ensure we fully saturate our sizeof() so we can reliably hash this struct as an array... > uint32_t fCap : 2; > uint32_t fJoin : 2; > uint32_t fOther : 1; > uint32_t fUnused : 32 - 5; I prefer this too, but it does require Jim to make sure fUnused is initialized. Folding those bits into the other fields (ideally with a note about how many bits they need) avoids this.
oops, I got lazy and didn't connect the int-sizes + bit-sizes. I think using uin16_t + bit counts is slightly more tricky to read than a consistent int-size across all of them. (e.g. uint32_t) I guess a lie about the required bit-size for each field is readable enough, so I can see not adding an fUnused field. I'm fixating on this mostly because 1) I like that we can re-use structs as parts of (hashable) keys, and 2) it is really tricky to know that our structs are clean (and therefore that the hasher is always deterministic). new suggestion: - use uint32_t everywhere - document in the last field that the bit-count is larger than actually needed - document all of it that we are shooting for a saturated struct for hashing
On 2015/11/20 at 16:08:35, reed wrote:
> oops, I got lazy and didn't connect the int-sizes + bit-sizes. I think using
uin16_t + bit counts is slightly more tricky to read than a consistent int-size
across all of them. (e.g. uint32_t)
>
> I guess a lie about the required bit-size for each field is readable enough,
so I can see not adding an fUnused field.
>
> I'm fixating on this mostly because 1) I like that we can re-use structs as
parts of (hashable) keys, and 2) it is really tricky to know that our structs
are clean (and therefore that the hasher is always deterministic).
>
> new suggestion:
> - use uint32_t everywhere
> - document in the last field that the bit-count is larger than actually needed
> - document all of it that we are shooting for a saturated struct for hashing
-Wpadded enforces this. We could turn it on and require all Skia types be
dense.
Or we can opt-in structs/classes at their definition site:
SkTypes.h:
// Can be used to bracket data types that must be dense, e.g. hash keys.
#if defined(__GNUC__)
#define SK_BEGIN_REQUIRE_DENSE _Pragma("GCC diagnostic push") \
_Pragma("GCC diagnostic error
\"-Wpadded\"")
#define SK_END_REQUIRE_DENSE _Pragma("GCC diagnostic pop")
#else
#define SK_BEGIN_REQUIRE_DENSE
#define SK_END_REQUIRE_DENSE
#endif
SomeCode.h:
SK_BEGIN_REQUIRE_DENSE
struct NotDense {
float f;
char c;
};
SK_END_REQUIRE_DENSE
--->
error: padding size of 'unpacked' with 3 bytes to alignment boundary
[-Werror,-Wpadded]
Updated the patch to incorporate comments. One thing: we still don't quite get equality between hashing and hasEqualEffect() since different values of SkStrokeRec can apply the same effect (fills in particular). When I use it as a key I ensure that when it's a fill it has a standard set of values (-1 width, butt cap, miter join, miter limit 4).
api lgtm
The CQ bit was checked by jvanverth@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1465773003/#ps80001 (title: "One more comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1465773003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1465773003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/897c9937636c2287bb217c76da9a56afb43050ac |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
