[Impeller] have Hostbuffer write directly to block allocated device buffers. #49505

jonahwilliams · 2024-01-03T21:09:52Z

We can't use the existing host buffer abstraction as that requires us to collect all allocations up front. By itself, this isn't sufficient for #140804 , because we'll need a way to mark ranges as dirty and/or flush if we don't have host coherent memory. But by itself this change should be beneficial as we'll create fewer device buffers and should do less allocation in general.

The size of the device buffers is 1024 Kb, somewhat arbitrarily chosen.

…uffers.

bdero · 2024-01-04T22:48:46Z

impeller/renderer/context.h

-  //----------------------------------------------------------------------------
-  /// @brief Accessor for a pool of HostBuffers.
-  Pool<HostBuffer>& GetHostBufferPool() const { return host_buffer_pool_; }
+  virtual const std::shared_ptr<HostBuffer> GetTransientsBuffer() const = 0;


Would it be problematic to track the transients buffer in the ContentContext and trigger per-frame lifecycle stuff from EntityPass::Render() (called once per frame on the root EntityPass, even when fancy Picture stuff is afoot)?

I think ContentContext would be a reasonable place to put this.

jonahwilliams · 2024-01-05T01:05:31Z

The problem with putting it on ContentContext is that we actually have a ton of test code that wants to use the transients buffer that only references context/render pass. So if I do that I have to refactor a bunch of test code :(

…_buffer

jonahwilliams · 2024-01-08T23:44:20Z

This is nearly working. i think we could land this without worrying about flushing until we switch to directly encoding to device buffers.

jonahwilliams · 2024-01-08T23:45:19Z

I mean command buffers.

jonahwilliams · 2024-01-09T00:04:18Z

Doesn't quite work for GLES yet, digging into that.

jonahwilliams · 2024-01-09T00:55:46Z

Okay, sort of fixed for GLES by making them initially dirty.

jonahwilliams · 2024-01-09T01:18:40Z

I still need to verify that this works with a Vulkan device that does not support host coherent memory. I have one on my desk and I'll check tomorrow.

jonahwilliams · 2024-01-09T01:28:05Z

This change also sort of breaks the flutter:gpu functions that bind to the host buffer.

jonahwilliams · 2024-01-09T01:31:55Z

basically you can no longer assume that the singular host buffer + offset is sufficient to reconstruct the correct buffer view. I think you'd either need to track some sort of token + the range, or else use a different abstraction that is separate from what impeller is using.

I could literally split them into two concepts, for flutter gpu you can have a host buffer that behaves like the old host buffer. And the rest of impeller can use a "transientArena" or something like that.

jonahwilliams · 2024-01-09T01:32:01Z

FYI @bdero

bdero · 2024-01-09T04:30:04Z

I pushed a solution to keep HostBuffer working in Flutter GPU here: #49618

bdero · 2024-01-09T04:51:51Z

impeller/entity/contents/content_context.h

@@ -940,6 +947,7 @@ class ContentContext {
  std::shared_ptr<scene::SceneContext> scene_context_;
 #endif  // IMPELLER_ENABLE_3D
  std::shared_ptr<RenderTargetAllocator> render_target_cache_;
+  std::shared_ptr<HostBuffer> host_buffer_;


bdero · 2024-01-09T05:02:33Z

impeller/core/device_buffer.h

@@ -45,6 +45,8 @@ class DeviceBuffer : public Buffer,

  virtual uint8_t* OnGetContents() const = 0;

+  virtual void Flush(std::optional<Range> range) const {}


Place definition in device_buffer.cc?

bdero · 2024-01-09T06:24:19Z

impeller/core/host_buffer.cc

+void HostBuffer::HostBufferState::MaybeCreateNewBuffer(size_t required_size) {
+  if (current_buffer + 1 >= device_buffers.size()) {
+    if (required_size > kAllocatorBlockSize) {
+      FML_LOG(ERROR) << "Created oversized buffer: " << required_size;


This can be addressed later when we start doing DeviceBuffer reuse, but it would probably be a good to track oversized DeviceBuffers separately from the block list. Otherwise one oversized draw could end up causing a few oversized blocked to get allocated while scrolling around, for example.

In the doc for flutter/flutter#138161 I recommended just throwing away oversized buffers and not tracking when starting out... But perhaps later we could form a heap for tracking oversize buffers sorted by size.

bdero · 2024-01-09T06:25:14Z

impeller/renderer/backend/vulkan/context_vk.h

@@ -198,6 +198,7 @@ class ContextVK final : public Context,
  std::unique_ptr<fml::Thread> queue_submit_thread_;
  std::shared_ptr<GPUTracerVK> gpu_tracer_;
  std::shared_ptr<DescriptorPoolRecyclerVK> descriptor_pool_recycler_;
+  std::shared_ptr<HostBuffer> host_buffer_;


Suggested change

std::shared_ptr<HostBuffer> host_buffer_;

bdero · 2024-01-09T06:25:23Z

impeller/renderer/backend/vulkan/context_vk.h

 #include "flutter/fml/mapping.h"
 #include "flutter/fml/unique_fd.h"
 #include "fml/thread.h"
 #include "impeller/base/backend_cast.h"
 #include "impeller/core/formats.h"
+#include "impeller/core/host_buffer.h"


Suggested change

#include "impeller/core/host_buffer.h"

bdero · 2024-01-09T06:28:05Z

impeller/renderer/backend/gles/context_gles.h

@@ -52,6 +53,7 @@ class ContextGLES final : public Context,
  std::shared_ptr<SamplerLibraryGLES> sampler_library_;
  std::shared_ptr<AllocatorGLES> resource_allocator_;
  std::shared_ptr<GPUTracerGLES> gpu_tracer_;
+  std::shared_ptr<HostBuffer> host_buffer_;


Suggested change

std::shared_ptr<HostBuffer> host_buffer_;

bdero · 2024-01-09T06:28:14Z

impeller/renderer/backend/gles/context_gles.h

@@ -9,6 +9,7 @@
 #include <unordered_map>
 #include "flutter/fml/macros.h"
 #include "impeller/base/backend_cast.h"
+#include "impeller/core/host_buffer.h"


Suggested change

#include "impeller/core/host_buffer.h"

bdero · 2024-01-09T06:30:59Z

impeller/entity/entity_pass.cc

+    renderer.GetRenderTargetCache()->End();
+    renderer.GetTransientsBuffer().Reset();
+#if IMPELLER_ENABLE_3D
+    renderer.GetSceneContext()->GetTransientsBuffer().Reset();


I think the best way to do this for Scene would be a scoped cleanup in Scene::Render instead?

bdero · 2024-01-09T06:33:03Z

impeller/renderer/backend/metal/context_mtl.h

@@ -129,6 +129,7 @@ class ContextMTL final : public Context,
 #endif  // IMPELLER_DEBUG
  std::deque<std::function<void()>> tasks_awaiting_gpu_;
  std::unique_ptr<SyncSwitchObserver> sync_switch_observer_;
+  std::shared_ptr<HostBuffer> host_buffer_;


Suggested change

std::shared_ptr<HostBuffer> host_buffer_;

bdero · 2024-01-09T06:42:52Z

impeller/core/host_buffer.cc

-  FML_CHECK(did_truncate);
+  offset = 0u;
+  current_buffer = 0u;
+  device_buffers.clear();


Any plans for reusing the DeviceBuffers across frames? One easy solution would be to rotate through MaxFramesInFlight vectors IMO.

Yeah that seems simple enough. Added this logic plus some trimming of buffers if they go unused.

bdero · 2024-01-09T19:29:53Z

lib/gpu/host_buffer.cc

@@ -13,7 +13,8 @@ namespace gpu {

 IMPLEMENT_WRAPPERTYPEINFO(flutter_gpu, HostBuffer);

-HostBuffer::HostBuffer() : host_buffer_(impeller::HostBuffer::Create()) {}
+HostBuffer::HostBuffer()
+    : host_buffer_(impeller::HostBuffer::Create(nullptr)) {}


To fix this in a sensible way I need to change the interface on the Dart side to be spawned from a a gpuContext. So feel free to land this break and I'll fix it later.

I took a shot at fixing that, seems to compile OK!

bdero

LGTM!

Make the wrapped HostBuffer wrapper track/look up emplacements using a fake byte offset. This is a trick to keep Flutter GPU working after flutter#49505 lands. I'll likely swing around and change how `BufferView` works later on. We can simplify a lot by making Flutter GPU `BufferView`s just take `DeviceBuffer` handles.

jonahwilliams · 2024-01-10T00:41:51Z

Incrementing the arena for host buffer allocations in EntityPass happened too frequently, in the event that there was any usage of toImage we could easy step on our own buffers, which happened in a few flutter tester tests. I've pushed a new design that lets content context reigster a callback for context to call every time Renderer::Render is called, and the reset is done there instead.

jonahwilliams · 2024-01-10T00:43:20Z

I do wonder if that will cause problems for the flutter tester style of rendering but I'm not certain what a reasonable fix would be for cases where we only ever call toImage.

jonahwilliams · 2024-01-10T00:50:44Z

impeller/entity/contents/content_context.cc

  if (!context_ || !context_->IsValid()) {
    return;
  }
+  // Register frame end callback to reset transient host buffer state.
+  context_->AddPerFrameCompleteTask(


🤷‍♂️ WDUT? @bdero @dnfield

Discussed real quick in person: We could trigger the reset call from AiksContext::Render since that'll happen per-frame

Jonah pointed out Picture::ToImage also calls AiksContext::Render, so going with bool to turn the resetting behavior off

jonahwilliams · 2024-01-10T16:18:30Z

Going to give this a shot now.

…d device buffers. (flutter/engine#49505)

…141291) flutter/engine@d1a2007...52aedc6 2024-01-10 jonahwilliams@google.com [Impeller] have Hostbuffer write directly to block allocated device buffers. (flutter/engine#49505) 2024-01-10 skia-flutter-autoroll@skia.org Roll Skia from 9271dcdade42 to 334160c0eede (1 revision) (flutter/engine#49675) 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 bdero@google.com,rmistry@google.com,zra@google.com 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

…device buffers. (#49505)" This reverts commit 52aedc6.

… device buffers." (#49688) Reverts #49505 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: part of flutter/flutter#140804 We can't use the existing host buffer abstraction as that requires us to collect all allocations up front. By itself, this isn't sufficient for #140804 , because we'll need a way to mark ranges as dirty and/or flush if we don't have host coherent memory. But by itself this change should be beneficial as we'll create fewer device buffers and should do less allocation in general. The size of the device buffers is 1024 Kb, somewhat arbitrarily chosen.

Reland of #49505 --- part of flutter/flutter#140804 We can't use the existing host buffer abstraction as that requires us to collect all allocations up front. By itself, this isn't sufficient for #140804 , because we'll need a way to mark ranges as dirty and/or flush if we don't have host coherent memory. But by itself this change should be beneficial as we'll create fewer device buffers and should do less allocation in general. The size of the device buffers is 1024 Kb, somewhat arbitrarily chosen.

[Impeller] have Hostbuffer write directly to block allocated device b…

340ebdd

…uffers.

github-actions bot added the e: impeller label Jan 3, 2024

jonahwilliams mentioned this pull request Jan 4, 2024

[Impeller] More efficient command encoding by removing deferred encoding. flutter/flutter#140804

Closed

more adjustments.

2cef26c

bdero reviewed Jan 4, 2024

View reviewed changes

jonahwilliams added 7 commits January 4, 2024 19:18

flush work.

2f04349

++

88d7b95

++

9ddcacd

oofta

9f896fb

move test file.

5a82b6e

Merge branch 'main' of github.com:flutter/engine into write_to_device…

723cc9b

…_buffer

reset transients in playground.

fefb257

GLES buffer starts out dirty.

3ab6e0f

jonahwilliams marked this pull request as ready for review January 9, 2024 00:55

++

7dd9854

bdero mentioned this pull request Jan 9, 2024

[Flutter GPU] Track HostBuffer emplacements by offset. #49618

Merged

bdero reviewed Jan 9, 2024

View reviewed changes

jonahwilliams added 3 commits January 9, 2024 09:43

cleanups, make flush work for GLES.

8c06294

fix const.

65dda4b

Add more unittests

b0d6e59

bdero reviewed Jan 9, 2024

View reviewed changes

bdero approved these changes Jan 9, 2024

View reviewed changes

fix fixtures.

b132dd7

jonahwilliams mentioned this pull request Jan 9, 2024

[Impeller] Frames in flight abstraction for HostBuffer may need to be app/platform dependenent. flutter/flutter#141201

Open

fix enable vk validations.

5a553f1

jonahwilliams added 2 commits January 9, 2024 15:16

fix tests and numerous off by ones.

d4a32ed

attempt to reset only on render frames.

2ccdb59

jonahwilliams commented Jan 10, 2024

View reviewed changes

jonahwilliams added 3 commits January 9, 2024 17:06

++

809adf5

add flag to aiks_context::Render to conditionally reset host buffers.

ac12014

Merge branch 'main' into write_to_device_buffer

3b1cbc5

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 10, 2024

auto-submit bot merged commit 52aedc6 into flutter:main Jan 10, 2024
28 checks passed

jonahwilliams deleted the write_to_device_buffer branch January 10, 2024 17:49

engine-flutter-autoroll mentioned this pull request Jan 10, 2024

Roll Flutter Engine from d1a2007a28b4 to 52aedc6c9153 (2 revisions) flutter/flutter#141291

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 10, 2024

52aedc6c9 [Impeller] have Hostbuffer write directly to block allocate…

05375df

…d device buffers. (flutter/engine#49505)

jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Jan 10, 2024

auto-submit bot pushed a commit that referenced this pull request Jan 10, 2024

Revert "[Impeller] have Hostbuffer write directly to block allocated …

cdb1afd

…device buffers. (#49505)" This reverts commit 52aedc6.

auto-submit bot mentioned this pull request Jan 10, 2024

Reverts "[Impeller] have Hostbuffer write directly to block allocated device buffers." #49688

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jan 10, 2024

jonahwilliams mentioned this pull request Jan 10, 2024

[Impeller] reland: write directly to device buffer. #49691

Merged

jonahwilliams mentioned this pull request Feb 11, 2024

[Impeller] Change the transient buffer to be a per-frame arena. flutter/flutter#138161

Closed

		@@ -45,6 +45,8 @@ class DeviceBuffer : public Buffer,

		virtual uint8_t* OnGetContents() const = 0;

		virtual void Flush(std::optional<Range> range) const {}

[Impeller] have Hostbuffer write directly to block allocated device buffers. #49505

[Impeller] have Hostbuffer write directly to block allocated device buffers. #49505

Conversation

jonahwilliams commented Jan 3, 2024 • edited Loading

bdero Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Jan 5, 2024

jonahwilliams commented Jan 8, 2024

jonahwilliams commented Jan 8, 2024

jonahwilliams commented Jan 9, 2024

jonahwilliams commented Jan 9, 2024

jonahwilliams commented Jan 9, 2024

jonahwilliams commented Jan 9, 2024

jonahwilliams commented Jan 9, 2024

jonahwilliams commented Jan 9, 2024

bdero commented Jan 9, 2024

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

bdero left a comment

Choose a reason for hiding this comment

jonahwilliams commented Jan 10, 2024

jonahwilliams commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Jan 10, 2024

jonahwilliams commented Jan 3, 2024 •

edited

Loading

bdero Jan 4, 2024 •

edited

Loading