Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1656143003:
Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. (Closed)
DescriptionMove Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files.
Does not change the public API.
TBR=reed
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1656143003
Committed: https://skia.googlesource.com/skia/+/c92159c8250c62cc47b7b63686538d61d54d2835
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Rename constants, update documentation. #
Messages
Total messages: 39 (18 generated)
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Description was changed from ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. BUG=skia: ========== to ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656143003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656143003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656143003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656143003/20001
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656143003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656143003/40001
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
benjaminwagner@google.com changed reviewers: + mtklein@google.com
After https://codereview.chromium.org/1513393002, I got tired of having to fix this every time it happens.
I like this approach. I'd also strongly approve an approach that sprinkled unconditional asserts like this into the appropriate data types: static_assert(N*sizeof(T) <= 4*1024, "Your stack allocation strategy is nutso... at >4K you're better off just using the heap!"); Most of the time when people add things like this, they don't really realize how large an allocation they're asking for. It may help to remind them to set their expectations realistically. https://codereview.chromium.org/1656143003/diff/40001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/1656143003/diff/40001/include/core/SkTypes.h#... include/core/SkTypes.h:614: template <size_t kSize> class SkAutoSMalloc : SkNoncopyable { Does this become simpler if we swap some names, renaming kSize kNominalSize and then make kSize calculated from that? https://codereview.chromium.org/1656143003/diff/40001/include/core/SkTypes.h#... include/core/SkTypes.h:689: // Align to 32 bits. Can't hurt to write // Align up to 32 bits. https://codereview.chromium.org/1656143003/diff/40001/include/core/SkTypes.h#... include/core/SkTypes.h:690: static const size_t kSizeAlign32 = ((kSize + 3) >> 2) << 2; We do have a nice little macro for this, static const size_ t kSizeAlign32 = SkAlign4(kSize); https://codereview.chromium.org/1656143003/diff/40001/include/private/SkTempl... File include/private/SkTemplates.h (right): https://codereview.chromium.org/1656143003/diff/40001/include/private/SkTempl... include/private/SkTemplates.h:198: if (fCount > N_ADJ) { Ditto here. It just seems easy to accidentally use N instead of N_ADJ. Let's make the user-supplied value have the weird name, e.g. N_REQUESTED, and the value we're supposed to use be the simple one, e.g. N.
reed@google.com changed reviewers: + reed@google.com
I think we have other places where we (naively) assume lots of stack-space when allocating temp storage. Perhaps this can evolve into something more general later.
The CQ bit was checked by benjaminwagner@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656143003/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656143003/50001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/02/02 at 03:04:29, mtklein wrote: > I like this approach. I'd also strongly approve an approach that sprinkled unconditional asserts like this into the appropriate data types: > > static_assert(N*sizeof(T) <= 4*1024, "Your stack allocation strategy is nutso... at >4K you're better off just using the heap!"); As much as I would like to add an assertion saying "mtklein sez you're nutso!", it sounds like reed would prefer changing compiler settings instead.
https://codereview.chromium.org/1656143003/diff/40001/include/core/SkTypes.h File include/core/SkTypes.h (right): https://codereview.chromium.org/1656143003/diff/40001/include/core/SkTypes.h#... include/core/SkTypes.h:614: template <size_t kSize> class SkAutoSMalloc : SkNoncopyable { On 2016/02/02 at 03:04:29, mtklein wrote: > Does this become simpler if we swap some names, renaming kSize kNominalSize and then make kSize calculated from that? Done. Since I had to update documentation, I rewrapped to 100 cols, per https://skia.org/dev/contrib/style. https://codereview.chromium.org/1656143003/diff/40001/include/core/SkTypes.h#... include/core/SkTypes.h:689: // Align to 32 bits. On 2016/02/02 at 03:04:29, mtklein wrote: > Can't hurt to write // Align up to 32 bits. Done. https://codereview.chromium.org/1656143003/diff/40001/include/core/SkTypes.h#... include/core/SkTypes.h:690: static const size_t kSizeAlign32 = ((kSize + 3) >> 2) << 2; On 2016/02/02 at 03:04:29, mtklein wrote: > We do have a nice little macro for this, > > static const size_ t kSizeAlign32 = SkAlign4(kSize); Done, x2. https://codereview.chromium.org/1656143003/diff/40001/include/private/SkTempl... File include/private/SkTemplates.h (right): https://codereview.chromium.org/1656143003/diff/40001/include/private/SkTempl... include/private/SkTemplates.h:198: if (fCount > N_ADJ) { On 2016/02/02 at 03:04:29, mtklein wrote: > Ditto here. It just seems easy to accidentally use N instead of N_ADJ. Let's make the user-supplied value have the weird name, e.g. N_REQUESTED, and the value we're supposed to use be the simple one, e.g. N. Done. I made the naming consistent with SkAutoSMalloc.
lgtm I might try removing the #ifdef GOOGLE3 parts as a follow up, to make this apply to all builds.
The CQ bit was checked by benjaminwagner@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656143003/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656143003/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
Description was changed from ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Does not change the public API. TBR=reed GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by benjaminwagner@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1656143003/50001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1656143003/50001
Message was sent while issue was closed.
Description was changed from ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Does not change the public API. TBR=reed GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Move Google3-specific stack limitation logic to template classes. Remove #ifdefs in other files. Does not change the public API. TBR=reed GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/c92159c8250c62cc47b7b63686538d61d54d2835 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:50001) as https://skia.googlesource.com/skia/+/c92159c8250c62cc47b7b63686538d61d54d2835
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:50001) has been created in https://codereview.chromium.org/1666503002/ by benjaminwagner@google.com. The reason for reverting is: See https://codereview.chromium.org/1665603002.
Message was sent while issue was closed.
On 2016/02/03 at 00:01:22, Ben Wagner wrote: > A revert of this CL (patchset #2 id:50001) has been created in https://codereview.chromium.org/1666503002/ by benjaminwagner@google.com. > > The reason for reverting is: See https://codereview.chromium.org/1665603002. Reapplying in https://codereview.chromium.org/1666203002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
