Bug 1452620 - Use FfiVec to clean up WrPipelineInfo usage in C++. r?nical,Gankro draft
authorKartikaya Gupta <kgupta@mozilla.com>
Tue, 10 Apr 2018 17:26:54 -0400
changeset 779948 2f7bb998a74aa5c2c6e7a2f57ca3a2a9992451ca
parent 779947 dcff36504495645bd51c366533156cc99ed1b8e0
child 779949 86cecebf97fe1c40af873c731ea94a59e53a975c
push id105919
push userkgupta@mozilla.com
push dateTue, 10 Apr 2018 21:27:12 +0000
reviewersnical, Gankro
bugs1452620
milestone61.0a1
Bug 1452620 - Use FfiVec to clean up WrPipelineInfo usage in C++. r?nical,Gankro Instead of passing a WrPipelineInfo* to C++ code, we can now pass the WrPipelineInfo directly. C++ code can access the info members without having to call back into rust code, so the wr_pipeline_info* iterator methods can be removed. Deleting the structure still requires passing it back to rust. MozReview-Commit-ID: HYcPX3mCGKW
gfx/layers/apz/public/APZUpdater.h
gfx/layers/apz/src/APZUpdater.cpp
gfx/webrender_bindings/RenderThread.cpp
gfx/webrender_bindings/RendererOGL.cpp
gfx/webrender_bindings/RendererOGL.h
gfx/webrender_bindings/src/bindings.rs
gfx/webrender_bindings/webrender_ffi.h
gfx/webrender_bindings/webrender_ffi_generated.h
--- a/gfx/layers/apz/public/APZUpdater.h
+++ b/gfx/layers/apz/public/APZUpdater.h
@@ -52,17 +52,17 @@ public:
    * This function is invoked from rust on the scene builder thread when it
    * is created. It effectively tells the APZUpdater "the current thread is
    * the updater thread for this window id" and allows APZUpdater to remember
    * which thread it is.
    */
   static void SetUpdaterThread(const wr::WrWindowId& aWindowId);
   static void PrepareForSceneSwap(const wr::WrWindowId& aWindowId);
   static void CompleteSceneSwap(const wr::WrWindowId& aWindowId,
-                                wr::WrPipelineInfo* aInfo);
+                                const wr::WrPipelineInfo& aInfo);
   static void ProcessPendingTasks(const wr::WrWindowId& aWindowId);
 
   void ClearTree(LayersId aRootLayersId);
   void UpdateFocusState(LayersId aRootLayerTreeId,
                         LayersId aOriginatingLayersId,
                         const FocusTarget& aFocusTarget);
   void UpdateHitTestingTree(LayersId aRootLayerTreeId,
                             Layer* aRoot,
--- a/gfx/layers/apz/src/APZUpdater.cpp
+++ b/gfx/layers/apz/src/APZUpdater.cpp
@@ -74,43 +74,40 @@ APZUpdater::PrepareForSceneSwap(const wr
 {
   if (RefPtr<APZUpdater> updater = GetUpdater(aWindowId)) {
     updater->mApz->LockTree();
   }
 }
 
 /*static*/ void
 APZUpdater::CompleteSceneSwap(const wr::WrWindowId& aWindowId,
-                              wr::WrPipelineInfo* aInfo)
+                              const wr::WrPipelineInfo& aInfo)
 {
   RefPtr<APZUpdater> updater = GetUpdater(aWindowId);
   if (!updater) {
     // This should only happen in cases where PrepareForSceneSwap also got a
     // null updater. No updater-thread tasks get run between PrepareForSceneSwap
     // and this function, so there is no opportunity for the updater mapping
     // to have gotten removed from sWindowIdMap in between the two calls.
     return;
   }
 
-  wr::WrPipelineId pipeline;
-  wr::WrEpoch epoch;
-  while (wr_pipeline_info_next_removed_pipeline(aInfo, &pipeline)) {
-    LayersId layersId = wr::AsLayersId(pipeline);
+  for (uintptr_t i = 0; i < aInfo.removed_pipelines.length; i++) {
+    LayersId layersId = wr::AsLayersId(aInfo.removed_pipelines.data[i]);
     updater->mEpochData.erase(layersId);
   }
   // Reset the built info for all pipelines, then put it back for the ones
   // that got built in this scene swap.
   for (auto& i : updater->mEpochData) {
     i.second.mBuilt = Nothing();
   }
-  while (wr_pipeline_info_next_epoch(aInfo, &pipeline, &epoch)) {
-    LayersId layersId = wr::AsLayersId(pipeline);
-    updater->mEpochData[layersId].mBuilt = Some(epoch);
+  for (uintptr_t i = 0; i < aInfo.epochs.length; i++) {
+    LayersId layersId = wr::AsLayersId(aInfo.epochs.data[i].pipeline_id);
+    updater->mEpochData[layersId].mBuilt = Some(aInfo.epochs.data[i].epoch);
   }
-  wr_pipeline_info_delete(aInfo);
 
   // Run any tasks that got unblocked, then unlock the tree. The order is
   // important because we want to run all the tasks up to and including the
   // UpdateHitTestingTree calls corresponding to the built epochs, and we
   // want to run those before we release the lock (i.e. atomically with the
   // scene swap). This ensures that any hit-tests always encounter a consistent
   // state between the APZ tree and the built scene in WR.
   //
@@ -520,21 +517,22 @@ apz_pre_scene_swap(mozilla::wr::WrWindow
 {
   // This should never get called unless async scene building is enabled.
   MOZ_ASSERT(gfxPrefs::WebRenderAsyncSceneBuild());
   mozilla::layers::APZUpdater::PrepareForSceneSwap(aWindowId);
 }
 
 void
 apz_post_scene_swap(mozilla::wr::WrWindowId aWindowId,
-                    mozilla::wr::WrPipelineInfo* aInfo)
+                    mozilla::wr::WrPipelineInfo aInfo)
 {
   // This should never get called unless async scene building is enabled.
   MOZ_ASSERT(gfxPrefs::WebRenderAsyncSceneBuild());
   mozilla::layers::APZUpdater::CompleteSceneSwap(aWindowId, aInfo);
+  wr_pipeline_info_delete(aInfo);
 }
 
 void
 apz_run_updater(mozilla::wr::WrWindowId aWindowId)
 {
   // This should never get called unless async scene building is enabled.
   MOZ_ASSERT(gfxPrefs::WebRenderAsyncSceneBuild());
   mozilla::layers::APZUpdater::ProcessPendingTasks(aWindowId);
--- a/gfx/webrender_bindings/RenderThread.cpp
+++ b/gfx/webrender_bindings/RenderThread.cpp
@@ -227,27 +227,29 @@ RenderThread::RunEvent(wr::WindowId aWin
   }
 
   aEvent->Run(*this, aWindowId);
   aEvent = nullptr;
 }
 
 static void
 NotifyDidRender(layers::CompositorBridgeParentBase* aBridge,
-                wr::WrPipelineInfo* aInfo,
+                wr::WrPipelineInfo aInfo,
                 TimeStamp aStart,
                 TimeStamp aEnd)
 {
-  wr::WrPipelineId pipeline;
-  wr::WrEpoch epoch;
-  while (wr_pipeline_info_next_epoch(aInfo, &pipeline, &epoch)) {
-    aBridge->NotifyDidCompositeToPipeline(pipeline, epoch, aStart, aEnd);
+  for (uintptr_t i = 0; i < aInfo.epochs.length; i++) {
+    aBridge->NotifyDidCompositeToPipeline(
+        aInfo.epochs.data[i].pipeline_id,
+        aInfo.epochs.data[i].epoch,
+        aStart,
+        aEnd);
   }
-  while (wr_pipeline_info_next_removed_pipeline(aInfo, &pipeline)) {
-    aBridge->NotifyPipelineRemoved(pipeline);
+  for (uintptr_t i = 0; i < aInfo.removed_pipelines.length; i++) {
+    aBridge->NotifyPipelineRemoved(aInfo.removed_pipelines.data[i]);
   }
 
   wr_pipeline_info_delete(aInfo);
 }
 
 void
 RenderThread::UpdateAndRender(wr::WindowId aWindowId, bool aReadback)
 {
--- a/gfx/webrender_bindings/RendererOGL.cpp
+++ b/gfx/webrender_bindings/RendererOGL.cpp
@@ -186,17 +186,17 @@ RendererOGL::SetFrameStartTime(const Tim
   if (mFrameStartTime) {
     // frame start time is already set. This could happen when multiple
     // generate frame requests are merged by webrender.
     return;
   }
   mFrameStartTime = aTime;
 }
 
-wr::WrPipelineInfo*
+wr::WrPipelineInfo
 RendererOGL::FlushPipelineInfo()
 {
   return wr_renderer_flush_pipeline_info(mRenderer);
 }
 
 RenderTextureHost*
 RendererOGL::GetRenderTexture(wr::WrExternalImageId aExternalImageId)
 {
--- a/gfx/webrender_bindings/RendererOGL.h
+++ b/gfx/webrender_bindings/RendererOGL.h
@@ -79,17 +79,17 @@ public:
 
   /// This can be called on the render thread only.
   bool Resume();
 
   layers::SyncObjectHost* GetSyncObject() const;
 
   layers::CompositorBridgeParentBase* GetCompositorBridge() { return mBridge; }
 
-  wr::WrPipelineInfo* FlushPipelineInfo();
+  wr::WrPipelineInfo FlushPipelineInfo();
 
   RenderTextureHost* GetRenderTexture(wr::WrExternalImageId aExternalImageId);
 
   wr::Renderer* GetRenderer() { return mRenderer; }
 
   gl::GLContext* gl() const;
 
 protected:
--- a/gfx/webrender_bindings/src/bindings.rs
+++ b/gfx/webrender_bindings/src/bindings.rs
@@ -625,75 +625,67 @@ pub extern "C" fn wr_renderer_current_ep
 /// cbindgen:postfix=WR_DESTRUCTOR_SAFE_FUNC
 #[no_mangle]
 pub unsafe extern "C" fn wr_renderer_delete(renderer: *mut Renderer) {
     let renderer = Box::from_raw(renderer);
     renderer.deinit();
     // let renderer go out of scope and get dropped
 }
 
+// cbindgen doesn't support tuples, so we have a little struct instead, with
+// an Into implementation to convert from the tuple to the struct.
+#[repr(C)]
+pub struct WrPipelineEpoch {
+    pipeline_id: WrPipelineId,
+    epoch: WrEpoch,
+}
+
+impl From<(WrPipelineId, WrEpoch)> for WrPipelineEpoch {
+    fn from(tuple: (WrPipelineId, WrEpoch)) -> WrPipelineEpoch {
+        WrPipelineEpoch {
+            pipeline_id: tuple.0,
+            epoch: tuple.1
+        }
+    }
+}
+
+#[repr(C)]
 pub struct WrPipelineInfo {
-    epochs: Vec<(WrPipelineId, WrEpoch)>,
-    removed_pipelines: Vec<PipelineId>,
+    epochs: FfiVec<WrPipelineEpoch>,
+    removed_pipelines: FfiVec<PipelineId>,
 }
 
 impl WrPipelineInfo {
     fn new(info: PipelineInfo) -> Self {
         WrPipelineInfo {
-            epochs: info.epochs.into_iter().collect(),
-            removed_pipelines: info.removed_pipelines,
+            epochs: FfiVec::from_vec(info.epochs.into_iter().map(WrPipelineEpoch::from).collect()),
+            removed_pipelines: FfiVec::from_vec(info.removed_pipelines),
         }
     }
 }
 
 #[no_mangle]
-pub unsafe extern "C" fn wr_renderer_flush_pipeline_info(renderer: &mut Renderer) -> *mut WrPipelineInfo {
+pub unsafe extern "C" fn wr_renderer_flush_pipeline_info(renderer: &mut Renderer) -> WrPipelineInfo {
     let info = renderer.flush_pipeline_info();
-    Box::into_raw(Box::new(WrPipelineInfo::new(info)))
-}
-
-#[no_mangle]
-pub unsafe extern "C" fn wr_pipeline_info_next_epoch(
-    info: &mut WrPipelineInfo,
-    out_pipeline: &mut WrPipelineId,
-    out_epoch: &mut WrEpoch
-) -> bool {
-    if let Some((pipeline, epoch)) = info.epochs.pop() {
-        *out_pipeline = pipeline;
-        *out_epoch = epoch;
-        return true;
-    }
-    return false;
-}
-
-#[no_mangle]
-pub unsafe extern "C" fn wr_pipeline_info_next_removed_pipeline(
-    info: &mut WrPipelineInfo,
-    out_pipeline: &mut WrPipelineId,
-) -> bool {
-    if let Some(pipeline) = info.removed_pipelines.pop() {
-        *out_pipeline = pipeline;
-        return true;
-    }
-    return false;
+    WrPipelineInfo::new(info)
 }
 
 /// cbindgen:postfix=WR_DESTRUCTOR_SAFE_FUNC
 #[no_mangle]
-pub unsafe extern "C" fn wr_pipeline_info_delete(info: *mut WrPipelineInfo) {
-    Box::from_raw(info);
+pub unsafe extern "C" fn wr_pipeline_info_delete(_info: WrPipelineInfo) {
+    // _info will be dropped here, and the drop impl on FfiVec will free
+    // the underlying vec memory
 }
 
-#[allow(improper_ctypes)] // this is needed so that rustc doesn't complain about passing the *mut WrPipelineInfo to an extern function
 extern "C" {
     fn apz_register_updater(window_id: WrWindowId);
     fn apz_pre_scene_swap(window_id: WrWindowId);
     // This function takes ownership of the pipeline_info and is responsible for
     // freeing it via wr_pipeline_info_delete.
-    fn apz_post_scene_swap(window_id: WrWindowId, pipeline_info: *mut WrPipelineInfo);
+    fn apz_post_scene_swap(window_id: WrWindowId, pipeline_info: WrPipelineInfo);
     fn apz_run_updater(window_id: WrWindowId);
     fn apz_deregister_updater(window_id: WrWindowId);
 }
 
 struct APZCallbacks {
     window_id: WrWindowId,
 }
 
@@ -710,17 +702,17 @@ impl SceneBuilderHooks for APZCallbacks 
         unsafe { apz_register_updater(self.window_id) }
     }
 
     fn pre_scene_swap(&self) {
         unsafe { apz_pre_scene_swap(self.window_id) }
     }
 
     fn post_scene_swap(&self, info: PipelineInfo) {
-        let info = Box::into_raw(Box::new(WrPipelineInfo::new(info)));
+        let info = WrPipelineInfo::new(info);
         unsafe { apz_post_scene_swap(self.window_id, info) }
     }
 
     fn poke(&self) {
         unsafe { apz_run_updater(self.window_id) }
     }
 
     fn deregister(&self) {
--- a/gfx/webrender_bindings/webrender_ffi.h
+++ b/gfx/webrender_bindings/webrender_ffi.h
@@ -88,17 +88,17 @@ struct FontInstanceFlags {
 struct WrWindowId;
 struct WrPipelineInfo;
 
 } // namespace wr
 } // namespace mozilla
 
 void apz_register_updater(mozilla::wr::WrWindowId aWindowId);
 void apz_pre_scene_swap(mozilla::wr::WrWindowId aWindowId);
-void apz_post_scene_swap(mozilla::wr::WrWindowId aWindowId, mozilla::wr::WrPipelineInfo* aInfo);
+void apz_post_scene_swap(mozilla::wr::WrWindowId aWindowId, mozilla::wr::WrPipelineInfo aInfo);
 void apz_run_updater(mozilla::wr::WrWindowId aWindowId);
 void apz_deregister_updater(mozilla::wr::WrWindowId aWindowId);
 
 } // extern "C"
 
 // Some useful defines to stub out webrender binding functions for when we
 // build gecko without webrender. We try to tell the compiler these functions
 // are unreachable in that case, but VC++ emits a warning if it finds any
--- a/gfx/webrender_bindings/webrender_ffi_generated.h
+++ b/gfx/webrender_bindings/webrender_ffi_generated.h
@@ -269,18 +269,16 @@ struct Transaction;
 struct UnknownUnit;
 
 template<typename T>
 struct Vec;
 
 // Geometry in the document's coordinate space (logical pixels).
 struct WorldPixel;
 
-struct WrPipelineInfo;
-
 struct WrProgramCache;
 
 struct WrState;
 
 struct WrThreadPool;
 
 struct IdNamespace {
   uint32_t mHandle;
@@ -324,16 +322,85 @@ struct WrWindowId {
   bool operator<(const WrWindowId& aOther) const {
     return mHandle < aOther.mHandle;
   }
   bool operator<=(const WrWindowId& aOther) const {
     return mHandle <= aOther.mHandle;
   }
 };
 
+// This type carries no valuable semantics for WR. However, it reflects the fact that
+// clients (Servo) may generate pipelines by different semi-independent sources.
+// These pipelines still belong to the same `IdNamespace` and the same `DocumentId`.
+// Having this extra Id field enables them to generate `PipelineId` without collision.
+using PipelineSourceId = uint32_t;
+
+// From the point of view of WR, `PipelineId` is completely opaque and generic as long as
+// it's clonable, serializable, comparable, and hashable.
+struct PipelineId {
+  PipelineSourceId mNamespace;
+  uint32_t mHandle;
+
+  bool operator==(const PipelineId& aOther) const {
+    return mNamespace == aOther.mNamespace &&
+           mHandle == aOther.mHandle;
+  }
+};
+
+using WrPipelineId = PipelineId;
+
+struct Epoch {
+  uint32_t mHandle;
+
+  bool operator==(const Epoch& aOther) const {
+    return mHandle == aOther.mHandle;
+  }
+  bool operator<(const Epoch& aOther) const {
+    return mHandle < aOther.mHandle;
+  }
+  bool operator<=(const Epoch& aOther) const {
+    return mHandle <= aOther.mHandle;
+  }
+};
+
+using WrEpoch = Epoch;
+
+struct WrPipelineEpoch {
+  WrPipelineId pipeline_id;
+  WrEpoch epoch;
+
+  bool operator==(const WrPipelineEpoch& aOther) const {
+    return pipeline_id == aOther.pipeline_id &&
+           epoch == aOther.epoch;
+  }
+};
+
+template<typename T>
+struct FfiVec {
+  const T *data;
+  uintptr_t length;
+  uintptr_t capacity;
+
+  bool operator==(const FfiVec& aOther) const {
+    return data == aOther.data &&
+           length == aOther.length &&
+           capacity == aOther.capacity;
+  }
+};
+
+struct WrPipelineInfo {
+  FfiVec<WrPipelineEpoch> epochs;
+  FfiVec<PipelineId> removed_pipelines;
+
+  bool operator==(const WrPipelineInfo& aOther) const {
+    return epochs == aOther.epochs &&
+           removed_pipelines == aOther.removed_pipelines;
+  }
+};
+
 template<typename T, typename U>
 struct TypedSize2D {
   T width;
   T height;
 
   bool operator==(const TypedSize2D& aOther) const {
     return width == aOther.width &&
            height == aOther.height;
@@ -391,36 +458,16 @@ struct TypedPoint2D {
   bool operator==(const TypedPoint2D& aOther) const {
     return x == aOther.x &&
            y == aOther.y;
   }
 };
 
 using WorldPoint = TypedPoint2D<float, WorldPixel>;
 
-// This type carries no valuable semantics for WR. However, it reflects the fact that
-// clients (Servo) may generate pipelines by different semi-independent sources.
-// These pipelines still belong to the same `IdNamespace` and the same `DocumentId`.
-// Having this extra Id field enables them to generate `PipelineId` without collision.
-using PipelineSourceId = uint32_t;
-
-// From the point of view of WR, `PipelineId` is completely opaque and generic as long as
-// it's clonable, serializable, comparable, and hashable.
-struct PipelineId {
-  PipelineSourceId mNamespace;
-  uint32_t mHandle;
-
-  bool operator==(const PipelineId& aOther) const {
-    return mNamespace == aOther.mNamespace &&
-           mHandle == aOther.mHandle;
-  }
-};
-
-using WrPipelineId = PipelineId;
-
 // A 2d Rectangle optionally tagged with a unit.
 template<typename T, typename U>
 struct TypedRect {
   TypedPoint2D<T, U> origin;
   TypedSize2D<T, U> size;
 
   bool operator==(const TypedRect& aOther) const {
     return origin == aOther.origin &&
@@ -754,32 +801,16 @@ struct MutByteSlice {
   uintptr_t len;
 
   bool operator==(const MutByteSlice& aOther) const {
     return buffer == aOther.buffer &&
            len == aOther.len;
   }
 };
 
-struct Epoch {
-  uint32_t mHandle;
-
-  bool operator==(const Epoch& aOther) const {
-    return mHandle == aOther.mHandle;
-  }
-  bool operator<(const Epoch& aOther) const {
-    return mHandle < aOther.mHandle;
-  }
-  bool operator<=(const Epoch& aOther) const {
-    return mHandle <= aOther.mHandle;
-  }
-};
-
-using WrEpoch = Epoch;
-
 struct WrDebugFlags {
   uint32_t mBits;
 
   bool operator==(const WrDebugFlags& aOther) const {
     return mBits == aOther.mBits;
   }
 };
 
@@ -945,17 +976,17 @@ extern void AddNativeFontHandle(WrFontKe
                                 void *aHandle,
                                 uint32_t aIndex);
 
 extern void DeleteFontData(WrFontKey aKey);
 
 extern void apz_deregister_updater(WrWindowId aWindowId);
 
 extern void apz_post_scene_swap(WrWindowId aWindowId,
-                                WrPipelineInfo *aPipelineInfo);
+                                WrPipelineInfo aPipelineInfo);
 
 extern void apz_pre_scene_swap(WrWindowId aWindowId);
 
 extern void apz_register_updater(WrWindowId aWindowId);
 
 extern void apz_run_updater(WrWindowId aWindowId);
 
 extern void gecko_printf_stderr_output(const char *aMsg);
@@ -1368,31 +1399,20 @@ extern void wr_notifier_external_event(W
 extern void wr_notifier_new_frame_ready(WrWindowId aWindowId);
 
 extern void wr_notifier_new_scroll_frame_ready(WrWindowId aWindowId,
                                                bool aCompositeNeeded);
 
 extern void wr_notifier_wake_up(WrWindowId aWindowId);
 
 WR_INLINE
-void wr_pipeline_info_delete(WrPipelineInfo *aInfo)
+void wr_pipeline_info_delete(WrPipelineInfo aInfo)
 WR_DESTRUCTOR_SAFE_FUNC;
 
 WR_INLINE
-bool wr_pipeline_info_next_epoch(WrPipelineInfo *aInfo,
-                                 WrPipelineId *aOutPipeline,
-                                 WrEpoch *aOutEpoch)
-WR_FUNC;
-
-WR_INLINE
-bool wr_pipeline_info_next_removed_pipeline(WrPipelineInfo *aInfo,
-                                            WrPipelineId *aOutPipeline)
-WR_FUNC;
-
-WR_INLINE
 void wr_program_cache_delete(WrProgramCache *aProgramCache)
 WR_DESTRUCTOR_SAFE_FUNC;
 
 WR_INLINE
 WrProgramCache *wr_program_cache_new()
 WR_FUNC;
 
 WR_INLINE
@@ -1401,17 +1421,17 @@ bool wr_renderer_current_epoch(Renderer 
                                WrEpoch *aOutEpoch)
 WR_FUNC;
 
 WR_INLINE
 void wr_renderer_delete(Renderer *aRenderer)
 WR_DESTRUCTOR_SAFE_FUNC;
 
 WR_INLINE
-WrPipelineInfo *wr_renderer_flush_pipeline_info(Renderer *aRenderer)
+WrPipelineInfo wr_renderer_flush_pipeline_info(Renderer *aRenderer)
 WR_FUNC;
 
 WR_INLINE
 WrDebugFlags wr_renderer_get_debug_flags(Renderer *aRenderer)
 WR_FUNC;
 
 WR_INLINE
 void wr_renderer_readback(Renderer *aRenderer,