Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1514503004:
SkBitmap move (Closed)
|
Created:
5 years ago by hal.canary Modified:
5 years ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkBitmap move
Running `Release/dm --gpu 0`, the number of times we call
SkBitmap::operator=(const SkBitmap&)
(which refs the pixelref) is reduced from ~214929 to ~214626.
Committed: https://skia.googlesource.com/skia/+/023bda0d836834eb1f98ceec15b169a492ab78ad
Patch Set 1 #Patch Set 2 : 2015-12-10 (Thursday) 17:55:13 EST #
Total comments: 2
Patch Set 3 : 2015-12-14 (Monday) 11:11:10 EST #Patch Set 4 : 2015-12-14 (Monday) 12:27:36 EST #
Total comments: 2
Patch Set 5 : reed@ style nit #Patch Set 6 : 2015-12-14 (Monday) 12:58:31 EST #Messages
Total messages: 28 (12 generated)
The CQ bit was checked by halcanary@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/1514503004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514503004/20001
halcanary@google.com changed reviewers: + mtklein@google.com
ptal
I don't have any objection to making SkBitmap moveable per se. I'm skeptical of using std::move anywhere it's not required or an important performance improvement. I'm not convinced any of these qualify (and even then, they all look like cases where NRVO makes move moot.) It's clearly not required. Which of these call sites are hot? https://codereview.chromium.org/1514503004/diff/20001/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/1514503004/diff/20001/src/core/SkBitmap.cpp#n... src/core/SkBitmap.cpp:44: memcpy(this, &other, sizeof(other)); Can't this just be this->swap(other)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SkBitmap move ========== to ========== SkBitmap move Running `Release/dm --gpu 0`, the number of times we call SkBitmap::operator=(const SkBitmap&) (which refs the pixelref) is reduced from ~214929 to ~214626. ==========
The CQ bit was checked by halcanary@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/1514503004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514503004/40001
The CQ bit was checked by halcanary@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/1514503004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514503004/60001
On 2015/12/10 at 23:09:58, mtklein wrote: > I'm skeptical of using std::move anywhere it's not required or an important performance improvement. I'm not convinced any of these qualify (and even then, they all look like cases where NRVO makes move moot.) It's clearly not required. Which of these call sites are hot? I removed all calls to std::move, since NRVO should work anyway. without std::move, running `Release/dm --gpu 0`, the number of times we call SkBitmap::operator=(const SkBitmap&) (which refs the pixelref) is reduced from ~214929 to ~214626.
https://codereview.chromium.org/1514503004/diff/20001/src/core/SkBitmap.cpp File src/core/SkBitmap.cpp (right): https://codereview.chromium.org/1514503004/diff/20001/src/core/SkBitmap.cpp#n... src/core/SkBitmap.cpp:44: memcpy(this, &other, sizeof(other)); On 2015/12/10 at 23:09:58, mtklein wrote: > Can't this just be this->swap(other)? Done. need to initialize all variables first.
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/1514503004/diff/60001/include/core/SkBitmap.h File include/core/SkBitmap.h (right): https://codereview.chromium.org/1514503004/diff/60001/include/core/SkBitmap.h... include/core/SkBitmap.h:69: SkBitmap(SkBitmap&&); style nit: move these closer to their counterparts above
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/1514503004/diff/60001/include/core/SkBitmap.h File include/core/SkBitmap.h (right): https://codereview.chromium.org/1514503004/diff/60001/include/core/SkBitmap.h... include/core/SkBitmap.h:69: SkBitmap(SkBitmap&&); On 2015/12/14 at 17:42:42, reed1 wrote: > style nit: move these closer to their counterparts above done
The CQ bit was checked by halcanary@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/1514503004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514503004/80001
lgtm
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1514503004/#ps100001 (title: "2015-12-14 (Monday) 12:58:31 EST")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1514503004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1514503004/100001
Message was sent while issue was closed.
Description was changed from ========== SkBitmap move Running `Release/dm --gpu 0`, the number of times we call SkBitmap::operator=(const SkBitmap&) (which refs the pixelref) is reduced from ~214929 to ~214626. ========== to ========== SkBitmap move Running `Release/dm --gpu 0`, the number of times we call SkBitmap::operator=(const SkBitmap&) (which refs the pixelref) is reduced from ~214929 to ~214626. Committed: https://skia.googlesource.com/skia/+/023bda0d836834eb1f98ceec15b169a492ab78ad ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/023bda0d836834eb1f98ceec15b169a492ab78ad |
