[Impeller] Move BufferView/Texture/Sampler when binding.

[jonahwilliams]

Most of the time we are passing an rvalue into this functions anyway, so in theory moving should be faster. Right?

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[gaaclarke]

This should probably be a std::unique_ptr too.

Imagine this case:

size_t Foo::bar() {
  BufferView view = GenerateBufferView();
  binder_->BindResource(stage_, slot_, metadata_, view);
  return view.GetSize();
}

The linter won't complain about an accidental copy because it is used after the copy. That wouldn't be possible with the std::unique_ptr.

size_t Foo::bar() {
  std::unique_ptr<BufferView> view = GenerateBufferView();
  binder_->BindResource(stage_, slot_, metadata_, std::move(view)); // move required
  return view->GetSize();  // crash!
}

[gaaclarke]

If I'm reading the code correctly, this used to be a copy here, but now is a move which should be faster.

[gaaclarke]

Do you just want to follow the pattern you had in the other PR of having explicit Clone for BufferView?

[jonahwilliams]

I do not want to use unique_ptr because adding more heap allocations is going to be counter productive to performance.

But making the type (mostly) move only seems like a good idea. I will give that a shot

[jonahwilliams]

I tried making it move only in #48630

The problem is that BufferView is placed directly on a VertexBuffer and directly in Command. So all of those types need to be made move only as wel. That's a bit more than I want to do at once, though making Cmd move only would eventually let us fix the style guide violation of add command - so its probably worth doing.

[gaaclarke]

LGTM assuming we have help from the linter if we introduce accidental copies and we are seeing performance benefits in some microbenchmarks.

The references made it impossible to get copies wrong, but also were suboptimal in the case where we could actually pass ownership. I'm willing to give this a shot.

[gaaclarke]

If you accidentally omit the std::move here, we get a linter failure right?

[jonahwilliams]

We do not :(

[jonahwilliams]

I think for move only, we should start with Command and work our way down, that would be easier than going bottom up.

[Hixie]

test-exempt: code refactor with no semantic change