[Impeller] Reduce extra allocation and shared_ptr ops during canvas recording.

[jonahwilliams]

Locally, this improves the performance of the canvas recording benchmarks which are tracked here. locally this is about 10-20% more ops.

https://flutter-engine-perf.skia.org/e/?queries=test%3DBM_CanvasRecord_draw_circle%26test%3DBM_CanvasRecord_draw_line%26test%3DBM_CanvasRecord_draw_rect&selected=commit%3D38397%26name%3D%252Cconfig%253Ddefault%252Ccpu_scaling_enabled%253Dfalse%252Cexecutable%253D_b_s_w_ir_cache_builder_src_out_host_release_canvas_benchmarks%252Clibrary_build_type%253Drelease%252Cmhz_per_cpu%253D2200%252Cnum_cpus%253D8%252Csub_result%253DTotalOpCount%252Ctest%253DBM_CanvasRecord_draw_line%252Cunit%253Dns%252C

There is still some usage of the other Paint helpers which I'd like to refactor out at a later time.

[jonahwilliams]

While this code uses shared_ptr its careful to move when possible to avoid the shared ptr release ops.

[gaaclarke]

No action required: rvalue parameters are technically forbidden ( https://google.github.io/styleguide/cppguide.html#Rvalue_references ) but it would have been nice to statically assert that people are moving geometry into this. I see there is just one case were you'd have to manually copy it to get an rvalue.

This is beyond the scope of this PR but we could have created a wrapper for std::shared_ptr that doesn't have a copy constructor.

template<typename T>
class  move_ptr {
  public:
    move_ptr(const ptr<T>&) = delete;
    move_ptr(const ptr<T>&&) = default;
    move_ptr<T> Clone() const {...}
  private:
   std::shared_ptr<T> ptr_;
};

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #48585 at sha fa80c7b

[jonahwilliams]

hmm, I think I was right and we are double applying CPU filters. Maybe the golden diff is because I fixed this . Need to investigate more.

[gaaclarke]

The logic looks good.

I think we should consider c++'s style guides for rvalue parameters and using std::unique_ptr for Entity. I'm fine punting on that, maybe we can decide as a team our policy on rvalue parameters.

You should eliminate the auto usage here though.

[gaaclarke]

No action required: rvalue parameters are technically forbidden ( https://google.github.io/styleguide/cppguide.html#Rvalue_references ) but it would have been nice to statically assert that people are moving geometry into this. I see there is just one case were you'd have to manually copy it to get an rvalue.

This is beyond the scope of this PR but we could have created a wrapper for std::shared_ptr that doesn't have a copy constructor.

template<typename T>
class  move_ptr {
  public:
    move_ptr(const ptr<T>&) = delete;
    move_ptr(const ptr<T>&&) = default;
    move_ptr<T> Clone() const {...}
  private:
   std::shared_ptr<T> ptr_;
};

[gaaclarke]

https://google.github.io/styleguide/cppguide.html#Type_deduction

If we are at the level where we are trying to optimize away superfluous shared_ptr copies having the actual type makes that easier to review. I'm not sure if a const auto& would be more appropriate. Hopefully our linters find that stuff now.

[gaaclarke]

This is technically forbidden by the style guide ( https://google.github.io/styleguide/cppguide.html#Rvalue_references). I know we've done it in the past and it makes total sense to me. I think the more googley solution would be to use std::unique_ptr. The benefit there is that it's hard for people to introduce accidental copies, but it adds an extra layer of indirection.

[jonahwilliams]

Interesting, I think that lint makes sense and I'm thinking of a few ways to do this better. i don't think we should Heap allocate the entity types, these are going into the heap allocated std::vector anyway and we probably don't need more indirection.

Maybe an API where a default entity is emplaced and then a reference returned so I can set values on it? then everything goes to the right memory, approximately. Does that make sense?

[gaaclarke]

Another option could be to delete the copy constructors for Entity and add move constructors. That would conform to the style guide and make sure people don't accidentally copy them. The downside is we might have to implement a Clone Function manually.

class Entity {
 public:
  Entity(const Entity&) = delete;
  Entity(const Entity&&) = default;
  Entity Clone() const;
};

The references thing makes sense too, but might be awkward.

[jonahwilliams]

I think its pretty rare that we are copying them (on purpose), so this seems like a better idea

[jonahwilliams]

actually it turns out we copy entities a lot!

See EntityPass::GetEntityForElement

[jonahwilliams]

I think I will back this change out for now

[jonahwilliams]

It turns out this was most of the performance improvement 🙃

[jonahwilliams]

Well, now its actually slower 😆

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48585 at sha f35c00b

[jonahwilliams]

Closing in favor of #48596