Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(13)

Issue 2899483002: cc: Audit and static_assert the types in PaintOpBuffer can be moved.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by danakj
Modified:
2 months, 2 weeks ago
Reviewers:
vmpstr, enne (OOO)
CC:
cc-bugs_chromium.org, chromium-reviews, weiliangc, yngve_vivaldi.com
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

cc: Audit and static_assert the types in PaintOpBuffer can be moved. This verifies that we can move the types stored in PaintOpBuffer to a new memory address without calling a move constructor or destructor, by doing memcpy() and free()ing the old memory. std::is_trivially_copyable() types satisfy this since they have a trivial move constructor, but it requires a lot of other things we do not require since we don't keep around both copies. std::is_trivially_move_constructible() should satisfy this also, as we are conceptually moving everything, but this is not available to us on all platforms right now. I have left a TODO to this effect. It actually provides more than we need as well, as it has built in the assumption that we will call the destructor on the source object. sk_sp<> is an example that does not satisfy this condition since it nulls out the moved-from object. Since we don't ever use/destroy the moved-from object this is not important for us, and sk_sp functions correctly when we memcpy() and free() it, essentially moving its memory address. Our requirements are that the object does not need to modify itself when changing its address. In practice this generally means that it owns no pointers to itself directly or indirectly. A bad example is that std::vector can have a pointer back to the vector from its internal buffer. As of this patch no types in PaintOpBuffer are problematic. But now all non-primitive types in PaintOps go through SafePaintOpData which does the following: - Adds static_asserts for is_trivially_copyable where possible. - Has explanations on other types that we hope will be maintained /o\. R=enne@chromium.org BUG=671433 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Patch Set 1 #

Patch Set 2 : check-copy better-comment #

Patch Set 3 : check-copy better-better-comment #

Total comments: 1

Patch Set 4 : check-copy . #

Patch Set 5 : check-copy . #

Total comments: 3

Patch Set 6 : check-copy skscalar #

Patch Set 7 : check-copy oopscompiles #

Patch Set 8 : check-copy some-comment-changes #

Patch Set 9 : check-copy changes #

Total comments: 2

Messages

Total messages: 56 (34 generated)
danakj
3 months ago (2017-05-19 20:31:05 UTC) #3
danakj
https://codereview.chromium.org/2899483002/diff/40001/cc/paint/paint_image.h File cc/paint/paint_image.h (right): https://codereview.chromium.org/2899483002/diff/40001/cc/paint/paint_image.h#newcode58 cc/paint/paint_image.h:58: // Because PaintImage is stored in PaintOpBuffer it must ...
3 months ago (2017-05-19 20:35:35 UTC) #10
enne (OOO)
I...have a lot of mixed feelings about this patch. I feel like the comments and ...
2 months, 3 weeks ago (2017-05-22 17:19:45 UTC) #13
danakj
On Mon, May 22, 2017 at 1:19 PM, enne@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > I...have ...
2 months, 3 weeks ago (2017-05-23 17:04:58 UTC) #14
enne (OOO)
On 2017/05/23 at 17:04:58, danakj wrote: > I could move it all to the top. ...
2 months, 3 weeks ago (2017-05-23 18:15:25 UTC) #15
danakj
On Tue, May 23, 2017 at 2:15 PM, enne@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > On ...
2 months, 3 weeks ago (2017-05-23 18:22:22 UTC) #16
danakj
PTAL
2 months, 2 weeks ago (2017-06-01 15:49:16 UTC) #19
enne (OOO)
https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h File cc/paint/paint_op_buffer.h (right): https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h#newcode802 cc/paint/paint_op_buffer.h:802: float degrees; If you think we should get rid ...
2 months, 2 weeks ago (2017-06-01 16:04:58 UTC) #25
danakj
https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h File cc/paint/paint_op_buffer.h (right): https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h#newcode802 cc/paint/paint_op_buffer.h:802: float degrees; On 2017/06/01 16:04:58, enne wrote: > If ...
2 months, 2 weeks ago (2017-06-01 18:01:18 UTC) #28
enne (OOO)
https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h File cc/paint/paint_op_buffer.h (right): https://codereview.chromium.org/2899483002/diff/80001/cc/paint/paint_op_buffer.h#newcode802 cc/paint/paint_op_buffer.h:802: float degrees; On 2017/06/01 at 18:01:17, danakj wrote: > ...
2 months, 2 weeks ago (2017-06-01 18:07:33 UTC) #29
vmpstr
Like your description says, I feel like this patch is adding static asserts for types ...
2 months, 2 weeks ago (2017-06-01 18:16:17 UTC) #31
danakj
On Thu, Jun 1, 2017 at 2:07 PM, enne@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > > ...
2 months, 2 weeks ago (2017-06-01 18:54:05 UTC) #32
danakj
On Thu, Jun 1, 2017 at 2:16 PM, <vmpstr@chromium.org> wrote: > Like your description says, ...
2 months, 2 weeks ago (2017-06-01 19:00:33 UTC) #33
vmpstr
On 2017/06/01 19:00:33, danakj wrote: > On Thu, Jun 1, 2017 at 2:16 PM, <mailto:vmpstr@chromium.org> ...
2 months, 2 weeks ago (2017-06-01 20:11:27 UTC) #40
danakj
On Thu, Jun 1, 2017 at 4:11 PM, <vmpstr@chromium.org> wrote: > On 2017/06/01 19:00:33, danakj ...
2 months, 2 weeks ago (2017-06-01 20:18:49 UTC) #41
vmpstr
> > You're right that this doesn't guard the internals of the types, and > ...
2 months, 2 weeks ago (2017-06-01 20:39:30 UTC) #42
danakj
On Thu, Jun 1, 2017 at 4:39 PM, <vmpstr@chromium.org> wrote: > > > > You're ...
2 months, 2 weeks ago (2017-06-01 21:08:40 UTC) #43
vmpstr
On 2017/06/01 21:08:40, danakj wrote: > On Thu, Jun 1, 2017 at 4:39 PM, <mailto:vmpstr@chromium.org> ...
2 months, 2 weeks ago (2017-06-01 21:26:40 UTC) #48
enne (OOO)
Another thought I had is that serialization logic subsumes some of this. Any type with ...
2 months, 2 weeks ago (2017-06-01 21:28:57 UTC) #49
danakj
On Thu, Jun 1, 2017 at 5:26 PM, <vmpstr@chromium.org> wrote: > On 2017/06/01 21:08:40, danakj ...
2 months, 2 weeks ago (2017-06-01 21:43:46 UTC) #50
danakj
I've uploaded those changes so far. On Thu, Jun 1, 2017 at 5:43 PM, <danakj+owner@chromium.org> ...
2 months, 2 weeks ago (2017-06-01 21:44:20 UTC) #52
vmpstr
2 months, 2 weeks ago (2017-06-01 22:00:55 UTC) #54
On 2017/06/01 21:43:46, danakj wrote:
> On Thu, Jun 1, 2017 at 5:26 PM, <mailto:vmpstr@chromium.org> wrote:
> 
> > On 2017/06/01 21:08:40, danakj wrote:
> >
> > > On Thu, Jun 1, 2017 at 4:39 PM, <mailto:vmpstr@chromium.org> wrote:
> > >
> > > > >
> > > > > You're right that this doesn't guard the internals of the types, and
> > > > > there's 2 ways I can think of to address that.
> > > > > 1) Comments as I've done for PaintImage.
> > > > > 2) Force types to expose something, and that thing can check the
> > ok-ness
> > > > of
> > > > > its members. This moves things down one more level.
> > > > >
> > > > > Would 2 do better by you?
> > > > >
> > > >
> > > > Since these are compile-time checks, you can just add static asserts
> > for
> > > > each
> > > > member of the corresponding types. Ie,
> > > >
> > > > class PaintFoo {
> > > > ...
> > > > private:
> > > > static_assert(base::is_trivially_copyable<Bar>::value, "");
> > > > Bar bar_;
> > > >
> > > > // base::is_trivially_copyable<sk_sp<Baz>>::value won't pass, but
> > it's ok,
> > > > because ...
> > > > sk_sp<Baz> baz_;
> > > > };
> > > >
> > > > That looks like it's a better protection than either 1 or 2 (I do want
> > 1
> > > > though).
> > > >
> > >
> > > This is the kinda verbosity we avoided in the PaintOps. I don't mind
> > doing
> > > this, but I wonder if we should move the template up and use it here too?
> > >
> > > something like cc::IsMemMovable? We could specialize on is_enum to avoid
> > > having to name them all for instance..
> > >
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > This came up when we were thinking of adding a vector of frames to
> > > > > > PaintImage,
> > > > > > which would violate the assumptions you've stated in
> > paint_op_buffer.
> > > > The
> > > > > > author
> > > > > > wasn't aware of these requirements, and honestly I wouldn't have
> > been
> > > > if it
> > > > > > wasn't for a comment in PaintImage. In other words, without the
> > > > comment in
> > > > > > PaintImage, there is nothing that would raise a red flag for
> > either the
> > > > > > author
> > > > > > or the reviewer. So, that comment is good. SafePaintOpData wouldn't
> > > > have
> > > > > > precluded the change though, since (1) it's in a seemingly
> > unrelated
> > > > file,
> > > > > > (2)
> > > > > > it would pass since it has no checks, and as a result (3) would
> > > > provide a
> > > > > > false
> > > > > > sense of security since it looks like we're checking something
> > with it,
> > > > > > and it
> > > > > > compiles, so looks like the change is good.
> > > > > >
> > > > >
> > > > > These checks as is only protect against adding things to PaintOp
> > > > directly,
> > > > > which I think we do want. We could call something on nested types
> > that we
> > > > > control, and have them recursively do the same sort of thing.
> > > >
> > > > For one I'm not sure how much we'd be adding to this. That is, a clip
> > rect
> > > > will
> > > > always have a rect kind of thing... Maybe you have some thoughts on
> > other
> > > > types
> > > > that would eventually be here.
> > > >
> > >
> > > Any data structure with a pointer in it could have this problem. Right
> > now
> > > they are all behind sk_sp<>s but in the future we will unwrap these to
> > make
> > > the stuff shared-memory-able. As we do so we need to look at the
> > internals
> > > of each type, and this is all a reminder to do so. Currently that's all
> > > wrapped up inside PaintFlags but we will unwrap that over time and move
> > > things out to the ops that use it, and drop refcounting for stuff. I can
> > > imagine intermediate states that have pointers and then we crash a lot
> > with
> > > confusing stacks.
> > >
> > > My goal is mostly to give reviewers (and authors) lots of context to make
> > > it harder to mess this up.
> >
> > I understand and share this goal. What I want in addition is to ensure
> > that the
> > author and the reviewer understand why we have this and realize that if
> > the code
> > compiles it doesn't mean that they haven't messed up. The patch as is
> > makes it
> > feel like as long as you can successfully compile, it's good.
> >
> 
> What do you think of how we could enforce the IsMemMovable for nested
> complex types we control? Do you like the idea of moving the template out
> and using it (we could add a specialization on T::kIsMemMovable then, to
> make it handle the recursion, with types declaring that along with using
> the template on their members. Then we wouldn't add an specialization for
> those types specifically, as they'd pass due to having the member.

Something along these lines works for me. It's essentially the same as a static
assert, but just wrapped into fewer lines.

> 
> 
> >
> >
> > >
> > >
> > > > This also looks like at best a hope that a person
> > > > adding something will notice most neighboring members are wrapped...
> > and
> > > > then
> > > > all they really have to do is add another specialization that doesn't
> > do
> > > > anything.
> > > >
> > >
> > > Yep, I depend on cargo culting and reviewers reading the file. I don't
> > have
> > > a better proposition tho.
> > >
> > >
> > > > I guess what I'm saying is that unless the reviewer is one of the
> > > > people that is already aware of these requirements, there are very few
> > > > guards to
> > > > hint at the requirements (save for maybe the comments).
> > > >
> > >
> > > I am hoping that there is enough context for a reviewer who is not aware
> > to
> > > become aware when looking at a patch here. Do you think that's not the
> > > case? Doing nothing would require them to be already aware. A comment
> > would
> > > give them a place to find it if they happened to. The templates will
> > force
> > > someone to write a new specialization beside a bunch of others that
> > explain
> > > things. At that point the reviewer gets a lot more to be like "hmm"
> > about,
> > > which is the goal.
> > >
> > >
> > > > Maybe at least don't specialize every type that we have, and have a
> > default
> > > > implementation that does a static assert (with a message). That will
> > > > trigger an
> > > > error of a new non trivially copyable type that will explain what to
> > do.
> > > >
> > >
> > > How do I not specialize each type, but still get an error for an unknown
> > > type?
> >
> > I'm not sure why you want an error if the type is trivially copyable. That
> > seems
> > like just extra effort on the author's part. I'm proposing a default
> > implementation that static asserts for this condition, so any previously
> > unknown
> > type will be checked and only fail if it's not trivially copyable. The
> > failure
> > should provide a link to some sort of documentation. The documentation, in
> > turn,
> > should explain why we require this condition and what criteria has to be
> > met
> > before the author writes an exception specialization (like for PaintImage
> > and
> >
> PaintFlags).
> >
> 
> Oh I see what you mean. I thought you meant a static_assert that would
> always fire in the default template. Done. Also used SafePaintOpData for
> the primitive types.
> 
> 
> 
> >
> >
> > >
> > >
> > > > Can you also explain why we have this requirement (near the block
> > where you
> > > > state that we do have this requirement).
> > > >
> > >
> > > // It is a requirement for every field in a PaintOp that we can memcpy it
> > > to
> > > // another place in memory and free the only one (without calling any
> > >
> > > // constructors or destructors).
> > >
> > > Is the comment on PaintOp. I will copy that roughly up to the template.
> > >
> > >
> > > > As it stands now, it's not clear what
> > > > the use case would be where the requirement is important.
> > > >
> > >
> > > Do you think that comment bit is not clear or just its position?
> >
> > The comment position is fine. I'm only saying that the comment is stating
> > that
> > we have an assumption, but doesn't seem to provide a reason why we need
> > that
> > assumption. I just want a "why" blurb not just "what".
> >
> 
> Ah, explaining that we realloc the buffer when it grows, gotcha. Done.
> 
> 
> >
> >
> > >
> > >
> > > > Furthermore, I think
> > > > it's important to note that simply memcpy-ing a ref counted pointer is
> > not
> > > > enough, the original copy _must_ be deleted without destructors. You
> > > > allude a
> > > > little bit about this in the comment, but I feel like it could use a
> > bit
> > > > more
> > > > WARNING type of things around.
> > > >
> > >
> > > // sk_sp<T> pointers behave if their memory address is changed
> > >
> > > Is the comment I have for sk_sp to explain that we're moving the address
> > of
> > > the object (memcpy+free) not memcpying. I can add some WARNING but I'm
> > not
> > > sure what to put. WARNING that we will free not just memcpy? That seems
> > to
> > > be strictly better though, as it's a moving address instead of skipping a
> > > constructor but having an object suddenly, so warning about that feels
> > > weird. Or would you like some WARNING in the PaintOp comment where I
> > > explain the whole thing? I'll throw a WARNING at the top of that and see
> > > what you think.
> > >
> >
> > I think if you add the "why" above it might become more clear. I want it
> > to be
> > very clear that you can't just memcpy the whole thing and then use both
> > copies,
> > since you're going to double free ref counted pointers.
> >
> 
> Ok I've added the way that we realloc the buffer. LMK if that's looking
> better.
> 

Yep that looks good. Thanks.

https://codereview.chromium.org/2899483002/diff/160001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2899483002/diff/160001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:81: struct SafePaintOpData;
Don't need the forward declare anymore

https://codereview.chromium.org/2899483002/diff/160001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:160: struct CC_PAINT_EXPORT PaintOp {
maybe we can add something like

template <typename T>
using Member = SafePaintOpData<T>::type;

to PaintOp and then everything can just be 

Member<bool> flag; type of things? AFAIK that would still trigger the static
assert?

(this is a nit, I'm just trying to improve the ergonomics of using this a bit,
but maybe this is more explicit. not sure)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b