Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Nov 27, 2024

issue: flutter/flutter#159177

The improvements come from using absl::flat_hash_map.

measurements with std vs absl (host_profile_arm64):

# Std
0.041124
0.0398002
0.0396546
0.0405552
0.0397742

# Absl
0.0353708
0.0336834
0.0338616
0.033050
0.0331976

Benchmark used

TEST(BufferBindingsGLESTest, BindUniformDataMicro) {
  BufferBindingsGLES bindings;
  absl::flat_hash_map<std::string, GLint> uniform_bindings;
  uniform_bindings["SHADERMETADATA.FOOBAR"] = 1;
  bindings.SetUniformBindings(std::move(uniform_bindings));
  std::shared_ptr<MockGLES> mock_gl = MockGLES::Init();
  MockAllocator allocator;
  Bindings vertex_bindings;

  ShaderMetadata shader_metadata = {
      .name = "shader_metadata",
      .members = {ShaderStructMemberMetadata{.type = ShaderType::kFloat,
                                             .name = "foobar",
                                             .offset = 0,
                                             .size = sizeof(float),
                                             .byte_length = sizeof(float)}}};
  std::shared_ptr<ReactorGLES> reactor;
  std::shared_ptr<Allocation> backing_store = std::make_shared<Allocation>();
  ASSERT_TRUE(backing_store->Truncate(Bytes{sizeof(float)}));
  DeviceBufferGLES device_buffer(DeviceBufferDescriptor{.size = sizeof(float)},
                                 reactor, backing_store);
  BufferView buffer_view(&device_buffer, Range(0, sizeof(float)));
  vertex_bindings.buffers.push_back(BufferAndUniformSlot{
      .slot =
          ShaderUniformSlot{
              .name = "foobar", .ext_res_0 = 0, .set = 0, .binding = 0},
      .view = BufferResource(&shader_metadata, buffer_view)});
  Bindings fragment_bindings;
  int32_t count = 5'000'000;
  auto start = std::chrono::high_resolution_clock::now();
  for (int32_t i = 0; i < count; ++i) {
    bindings.BindUniformData(mock_gl->GetProcTable(), allocator,
                             vertex_bindings, fragment_bindings);
    if (i % 100 == 0) {
      mock_gl->GetCapturedCalls();
    }
  }
  auto end = std::chrono::high_resolution_clock::now();
  auto duration =
      std::chrono::duration_cast<std::chrono::microseconds>(end - start)
          .count();
  std::cout << "Execution time: " << duration / static_cast<double>(count)
            << " microseconds" << std::endl;
}

This is one of our hottest symbols on the raster thread:

Screenshot 2024-11-27 at 2 27 54 PM

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 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.

@gaaclarke
Copy link
Member Author

We are planning on refactoring this at some point, in the meantime this is an easy win in case that doesn't pan out.

gaaclarke added a commit that referenced this pull request Dec 2, 2024
We don't need consistent pointers anymore since we are avoiding deleting
objects to minimize time in the writer lock

related pr with benchmarks: #56844

## Pre-launch Checklist

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

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
.size = sizeof(float),
.byte_length = sizeof(float)}}};
std::shared_ptr<ReactorGLES> reactor;
std::shared_ptr<Allocation> backing_store = std::make_shared<Allocation>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're using a real allocator here I would use this in place of the mock allocator above. In fact, the transients_allocator arg is actually unused so you could also just delete it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 2, 2024
@gaaclarke gaaclarke merged commit 96eca99 into flutter:main Dec 2, 2024
33 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 2, 2024
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Dec 2, 2024
…159694)

flutter/engine@e0900f8...05fdaa6

2024-12-02 [email protected] Roll Skia from d7751d3d6ff4 to
2c4ce1d953ba (5 revisions) (flutter/engine#56901)
2024-12-02 [email protected] [ci] use env for format and support
arm64 hosts (flutter/engine#56268)
2024-12-02 [email protected] Replace outdated wiki URLs in pull
request template (flutter/engine#56824)
2024-12-02 [email protected] [impeller] makes
UniformBindData 15% faster and adds unit test (flutter/engine#56844)
2024-12-02 [email protected] Roll Skia from dd70c8e1c38f to
d7751d3d6ff4 (36 revisions) (flutter/engine#56898)
2024-12-02 [email protected] Roll Dart SDK from
5785058c9bb1 to 46630dbefbbd (12 revisions) (flutter/engine#56897)
2024-12-02 [email protected] switched reactor
to absl::flat_hash_map (flutter/engine#56845)
2024-12-02 [email protected] Sped up
SubpixelGlyph::Equal (flutter/engine#56851)
2024-12-02 [email protected] [Impeller] dont cache glyph layout
if atlas is invalid. (flutter/engine#56879)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit bot pushed a commit that referenced this pull request Dec 3, 2024
In #56844 we saw a 15% increase in performance by switching to absl::flat_hash_map.  I suspect we say see even better results ([source](https://martin.ankerl.com/2022/08/27/hashmap-bench-01)).  The absl guidance is to default to using this.

test exempt: no intentional functional change, just performance

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
We don't need consistent pointers anymore since we are avoiding deleting
objects to minimize time in the writer lock

related pr with benchmarks: flutter/engine#56844

## Pre-launch Checklist

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

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
…er/engine#56844)

issue: flutter#159177

The improvements come from using absl::flat_hash_map.

measurements with std vs absl (`host_profile_arm64`):
```
# Std
0.041124
0.0398002
0.0396546
0.0405552
0.0397742

# Absl
0.0353708
0.0336834
0.0338616
0.033050
0.0331976
```

Benchmark used
```c++
TEST(BufferBindingsGLESTest, BindUniformDataMicro) {
  BufferBindingsGLES bindings;
  absl::flat_hash_map<std::string, GLint> uniform_bindings;
  uniform_bindings["SHADERMETADATA.FOOBAR"] = 1;
  bindings.SetUniformBindings(std::move(uniform_bindings));
  std::shared_ptr<MockGLES> mock_gl = MockGLES::Init();
  MockAllocator allocator;
  Bindings vertex_bindings;

  ShaderMetadata shader_metadata = {
      .name = "shader_metadata",
      .members = {ShaderStructMemberMetadata{.type = ShaderType::kFloat,
                                             .name = "foobar",
                                             .offset = 0,
                                             .size = sizeof(float),
                                             .byte_length = sizeof(float)}}};
  std::shared_ptr<ReactorGLES> reactor;
  std::shared_ptr<Allocation> backing_store = std::make_shared<Allocation>();
  ASSERT_TRUE(backing_store->Truncate(Bytes{sizeof(float)}));
  DeviceBufferGLES device_buffer(DeviceBufferDescriptor{.size = sizeof(float)},
                                 reactor, backing_store);
  BufferView buffer_view(&device_buffer, Range(0, sizeof(float)));
  vertex_bindings.buffers.push_back(BufferAndUniformSlot{
      .slot =
          ShaderUniformSlot{
              .name = "foobar", .ext_res_0 = 0, .set = 0, .binding = 0},
      .view = BufferResource(&shader_metadata, buffer_view)});
  Bindings fragment_bindings;
  int32_t count = 5'000'000;
  auto start = std::chrono::high_resolution_clock::now();
  for (int32_t i = 0; i < count; ++i) {
    bindings.BindUniformData(mock_gl->GetProcTable(), allocator,
                             vertex_bindings, fragment_bindings);
    if (i % 100 == 0) {
      mock_gl->GetCapturedCalls();
    }
  }
  auto end = std::chrono::high_resolution_clock::now();
  auto duration =
      std::chrono::duration_cast<std::chrono::microseconds>(end - start)
          .count();
  std::cout << "Execution time: " << duration / static_cast<double>(count)
            << " microseconds" << std::endl;
}
```

This is one of our hottest symbols on the raster thread:

<img width="1528" alt="Screenshot 2024-11-27 at 2 27 54 PM"
src="https://github.com/user-attachments/assets/a9029d6f-ee96-4612-83f1-6b69f24e6ce8">

## Pre-launch Checklist

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

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
In flutter/engine#56844 we saw a 15% increase in performance by switching to absl::flat_hash_map.  I suspect we say see even better results ([source](https://martin.ankerl.com/2022/08/27/hashmap-bench-01)).  The absl guidance is to default to using this.

test exempt: no intentional functional change, just performance

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants