Removed instances of unnecessary values #36221

gaaclarke · 2022-09-16T21:40:27Z

This removes superfluous copies, improves locality and takes advantages of moves when possible. It also adds a linter check too and removed superfluous moves of const variables.

Most of these changes are automated fixes from clang-tidy. I've reviewed the changes and fixed them where necessary. In aggregate they may cause a significant shift in performance of the engine. Many of the copy removals are in code that is hardly used, but others are high trafficked.

The big changes are:

Eliminating copies of shared_ptr - this is expensive because it has a mutex
Eliminating copies of sk_pr - does this have a mutex?
Eliminating copies of closures - this can mask copies of captured variables
Eliminating copies of vectors - I saw this a handful of places as well
Eliminating RefPtr copies - this constructor is lightweight but we get locality benefits

There were too many instances to fix in one PR though. I tried to get them all but even this incomplete run is enough. I'll have to run clang-tidy with a few different variations of build files. I see there are a few impeller files that were missed for example that look important.

For testing I propose we look at skiaperf after this has landed to measure the impact. It should not hurt performance, but it also may not have a big impact on our performance tests.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

copies). Adds linter check too.

gaaclarke · 2022-09-16T23:20:58Z

So I double checked, this addresses all the issues I had in my current build target. Clang-tidy works based on your current build setup so to eliminate the other instances I'd have to run clang-tidy with those targets setup. I can do that in a different PR.

gaaclarke

Here are a few instances that I think are worthwhile in particular.

gaaclarke · 2022-09-16T23:44:19Z

common/graphics/texture.h

@@ -62,7 +62,7 @@ class TextureRegistry {
  TextureRegistry();

  // Called from raster thread.
-  void RegisterTexture(std::shared_ptr<Texture> texture);


This is potentially significant.

gaaclarke · 2022-09-16T23:47:53Z

impeller/renderer/command.h

@@ -153,17 +153,17 @@ struct Command {
  bool BindResource(ShaderStage stage,
                    const ShaderUniformSlot& slot,
                    const ShaderMetadata& metadata,
-                    BufferView view);


These are potentially big (BufferView has a shared_ptr in it too).

Buffer view copies don't copy the buffer. They just have a pointer to the buffer.

gaaclarke · 2022-09-16T23:48:12Z

impeller/renderer/command_buffer.h

@@ -61,7 +61,7 @@ class CommandBuffer {
  ///
  /// @param[in]  callback  The completion callback.
  ///
-  [[nodiscard]] bool SubmitCommands(CompletionCallback callback);


potentially big

gaaclarke · 2022-09-16T23:48:57Z

impeller/renderer/renderer.h

@@ -31,7 +31,8 @@ class Renderer {

  bool IsValid() const;

-  bool Render(std::unique_ptr<Surface> surface, RenderCallback callback) const;


potentially big

gaaclarke · 2022-09-16T23:50:15Z

impeller/typographer/text_run.h

@@ -31,7 +31,7 @@ class TextRun {
  ///
  /// @param[in]  font  The font
  ///
-  TextRun(Font font);


This sounds big. I'm not familiar with the code.

gaaclarke · 2022-09-16T23:51:53Z

lib/ui/semantics/semantics_update_builder.h

@@ -29,41 +29,42 @@ class SemanticsUpdateBuilder

  ~SemanticsUpdateBuilder() override;

-  void updateNode(int id,


seems significant too

gaaclarke · 2022-09-16T23:52:57Z

lib/ui/window/viewport_metrics.h

@@ -32,9 +32,9 @@ struct ViewportMetrics {
                  double p_physical_system_gesture_inset_bottom,
                  double p_physical_system_gesture_inset_left,
                  double p_physical_touch_slop,
-                  const std::vector<double> p_physical_display_features_bounds,


This may happen in an animation no? these vectors shouldn't be that big though.

gaaclarke · 2022-09-16T23:54:36Z

shell/common/rasterizer.h

@@ -256,7 +256,7 @@ class Rasterizer final : public SnapshotDelegate,
  /// @param[in]  discard_callback if specified and returns true, the layer tree
  ///                             is discarded instead of being rendered
  ///
-  RasterStatus Draw(std::shared_ptr<LayerTreePipeline> pipeline,


This looks big.

gaaclarke · 2022-09-16T23:56:01Z

shell/platform/android/external_view_embedder/surface_pool.h

@@ -55,8 +55,8 @@ class SurfacePool {
  std::shared_ptr<OverlayLayer> GetLayer(
      GrDirectContext* gr_context,
      const AndroidContext& android_context,
-      std::shared_ptr<PlatformViewAndroidJNI> jni_facade,


seems impactful

gaaclarke · 2022-09-16T23:56:30Z

shell/platform/android/platform_view_android_delegate/platform_view_android_delegate.h

@@ -18,8 +18,9 @@ class PlatformViewAndroidDelegate {
 public:
  explicit PlatformViewAndroidDelegate(
      std::shared_ptr<PlatformViewAndroidJNI> jni_facade);
-  void UpdateSemantics(flutter::SemanticsNodeUpdates update,


I'm not sure what these classes are, but might be something here.

dnfield

These LGTM, would appreciate review from Jason and Chinmay too

chinmaygarde · 2022-09-17T20:46:53Z

impeller/entity/inline_pass_context.h

@@ -18,7 +18,7 @@ class InlinePassContext {
  };

  InlinePassContext(std::shared_ptr<Context> context,
-                    RenderTarget render_target,
+                    const RenderTarget& render_target,


This introduces a copy where one could have been elided before. That is, if the caller had moved in a render target. Now, the const reference necessitates a copy.

It doesn't necessitate one: https://godbolt.org/z/vEbY44YMP

Clang can just ignore the move.

chinmaygarde · 2022-09-17T20:47:18Z

impeller/entity/inline_pass_context.cc

                                     uint32_t pass_texture_reads)
-    : context_(context),
+    : context_(std::move(context)),


This is is now a copy. The right fix here is to add the move but remove the const reference in the argument.

For context? It went from 2 copies to 1 copy and one move. render_target is correct, that too went form 2 copies to one. When there is a custom copy constructor those shouldn't be able to be optimized away (edit, actually maybe it can, it does in the return case).

chinmaygarde · 2022-09-17T20:47:49Z

impeller/renderer/backend/gles/allocator_gles.cc

@@ -40,7 +40,7 @@ std::shared_ptr<DeviceBuffer> AllocatorGLES::OnCreateBuffer(
 // |Allocator|
 std::shared_ptr<Texture> AllocatorGLES::OnCreateTexture(
    const TextureDescriptor& desc) {
-  return std::make_shared<TextureGLES>(reactor_, std::move(desc));
+  return std::make_shared<TextureGLES>(reactor_, desc);


Moving a const reference does nothing.

Yea, I also added a flag that removes moves of const.

chinmaygarde · 2022-09-17T20:48:23Z

impeller/renderer/backend/metal/command_buffer_mtl.h

@@ -21,7 +21,7 @@ class CommandBufferMTL final : public CommandBuffer {

  id<MTLCommandBuffer> buffer_ = nullptr;

-  CommandBufferMTL(const std::weak_ptr<const Context> context,
+  CommandBufferMTL(const std::weak_ptr<const Context>& context,


Good catch.

chinmaygarde · 2022-09-17T20:48:55Z

impeller/renderer/backend/metal/texture_mtl.mm

@@ -9,7 +9,7 @@
 namespace impeller {

 TextureMTL::TextureMTL(TextureDescriptor p_desc, id<MTLTexture> texture)
-    : Texture(std::move(p_desc)), texture_(texture) {
+    : Texture(p_desc), texture_(texture) {


Good catch.

chinmaygarde

Copies of shared pointers don't acquire a mutex. They are just atomic bumps of control block reference counts. Also, at least some of the changes (I've highlighted a couple) make copy elision impossible even if the caller cooperated.

There are other changes in here that are useful though.

IMO, this patch is doing too much. Can we divvy it up into smaller chunks so its easier to review. Especially since this doens't seem to be a mechanical change thats always desirable to do as a matter of policy.

If its possible to gather numbers across all existing benchmarks after this change though, I love to see those before this lands. The speculative nature of this LSC makes me nervous.

gaaclarke · 2022-09-19T16:36:39Z

Copies of shared pointers don't acquire a mutex. They are just atomic bumps of control block reference counts. Also, at least some of the changes (I've highlighted a couple) make copy elision impossible even if the caller cooperated.

Yea, you are right that they moved to std::atomic. I clearly haven't looked at shared_ptr source in a while.

With copy elision I think it's better to be explicit in the code than to rely on magic that may or may not happy in certain cases. We are using Microsoft's c++ compiler and standard library too on Windows no? So we have to consider their output as well which gets tricky.

There are other changes in here that are useful though.

IMO, this patch is doing too much. Can we divvy it up into smaller chunks so its easier to review. Especially since this doens't seem to be a mechanical change thats always desirable to do as a matter of policy.

If its possible to gather numbers across all existing benchmarks after this change though, I love to see those before this lands. The speculative nature of this LSC makes me nervous.

Sounds good, do you have a recommendation on what would be a good benchmark to see?

gaaclarke · 2022-09-19T16:41:11Z

@chinmaygarde there is one more thing to consider too. This change is affecting the linter so that potentially guards us from performance errors in the future as well. So it isn't just about making what we have today faster, but also making sure we don't introduce an accidental value that has a negative effect on our performance.

gaaclarke · 2022-09-26T17:31:51Z

It was decided in a PR review meeting that PR's that make the codebase more consistent and cleaner do not need to prove performance gains.

gaaclarke · 2022-09-26T18:39:14Z

@chinmaygarde I merged master with the following method. Can you please give this a quick look before it atrophies given our decision from the PR review meeting decision.

How to update:

Merge upstream/main
Checkout current for all merge conflicts
Build the target and fix any build problems
Run clang-tidy fix again

gaaclarke · 2022-09-26T19:02:20Z

impeller/typographer/lazy_glyph_atlas.cc

@@ -17,7 +17,7 @@ LazyGlyphAtlas::~LazyGlyphAtlas() = default;
 void LazyGlyphAtlas::AddTextFrame(const TextFrame& frame) {
  FML_DCHECK(atlas_map_.empty());
  has_color_ |= frame.HasColor();
-  frames_.emplace_back(std::move(frame));
+  frames_.emplace_back(frame);


I was a bit surprised this works, but it does, fun c++.

gaaclarke · 2022-09-26T20:30:12Z

The last failure was human error. Any clang-tidy fix that compiles but fails at execution time would be grounds to abandon this change, but that wasn't the case.

flutter-dashboard bot added embedder Related to the embedder API platform-android labels Sep 16, 2022

gaaclarke force-pushed the unnecessary-value branch 4 times, most recently from 885b04f to 00bf037 Compare September 16, 2022 22:16

Removed instances of unnecessary values (eliminates superfluous

a3cb39c

copies). Adds linter check too.

gaaclarke force-pushed the unnecessary-value branch from 00bf037 to a3cb39c Compare September 16, 2022 22:47

gaaclarke marked this pull request as ready for review September 16, 2022 23:04

gaaclarke requested review from dnfield and zanderso September 16, 2022 23:04

dnfield requested review from jason-simmons and chinmaygarde September 16, 2022 23:17

gaaclarke changed the title ~~Removed instances of unnecessary values (eliminates superfluous copies)~~ Removed instances of unnecessary values Sep 16, 2022

gaaclarke commented Sep 16, 2022

View reviewed changes

dnfield approved these changes Sep 17, 2022

View reviewed changes

chinmaygarde reviewed Sep 17, 2022

View reviewed changes

chinmaygarde requested changes Sep 17, 2022

View reviewed changes

gaaclarke added 2 commits September 26, 2022 11:15

Merge remote-tracking branch 'upstream/main' into unnecessary-value

0e7e00c

brought over fixes that got lost in the merge

2038c0a

gaaclarke commented Sep 26, 2022

View reviewed changes

fixed typo

72ce1f4

chinmaygarde approved these changes Sep 26, 2022

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2022

auto-submit bot merged commit 8fdb92c into flutter:main Sep 26, 2022

engine-flutter-autoroll mentioned this pull request Sep 26, 2022

Roll Flutter Engine from 349c0647b4be to e6cc7ddaaf0f (4 revisions) flutter/flutter#112420

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 26, 2022

8fdb92cd9 Removed instances of unnecessary values (flutter/engine#36221)

75ce7c1

zanderso mentioned this pull request Sep 28, 2022

//flutter/ci/lint.sh lints all files instead of just the ones modified in the patch. flutter/flutter#105068

Closed

Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022

Removed instances of unnecessary values (flutter#36221)

05e22e6

This was referenced Oct 20, 2022

Only run clang-tidy warning checks reported as errors #36885

Merged

Add performance-move-const-arg and performance-unnecessary-value-param checks to clang-tidy flutter/flutter#113804

Closed

zanderso mentioned this pull request Aug 17, 2023

[Engine] Audit clang_tidy --enable-check-profile and see what is expensive flutter/flutter#132687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed instances of unnecessary values #36221

Removed instances of unnecessary values #36221

		@@ -31,7 +31,8 @@ class Renderer {

		bool IsValid() const;

		bool Render(std::unique_ptr<Surface> surface, RenderCallback callback) const;

		@@ -29,41 +29,42 @@ class SemanticsUpdateBuilder

		~SemanticsUpdateBuilder() override;

		void updateNode(int id,

Removed instances of unnecessary values #36221

Removed instances of unnecessary values #36221

Conversation

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment