Bug 1499186 Part 2 - Only allow one in flight paint at a time, r=nical.
authorBrian Hackett <bhackett1024@gmail.com>
Wed, 17 Oct 2018 10:32:13 -0600
changeset 500508 b341213ecf95d1448abc0a38ef058138b41324ee
parent 500507 0fba5665bc65c55a4d1f27905a2a60396dbe2bb8
child 500509 7400b733b50f3a9d4fa8ca18363a9dbb52956398
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnical
bugs1499186
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1499186 Part 2 - Only allow one in flight paint at a time, r=nical.
toolkit/recordreplay/ipc/Channel.h
toolkit/recordreplay/ipc/ChildIPC.cpp
toolkit/recordreplay/ipc/ChildInternal.h
toolkit/recordreplay/ipc/ChildNavigation.cpp
toolkit/recordreplay/ipc/DisabledIPC.cpp
toolkit/recordreplay/ipc/JSControl.cpp
--- a/toolkit/recordreplay/ipc/Channel.h
+++ b/toolkit/recordreplay/ipc/Channel.h
@@ -373,21 +373,25 @@ struct FatalErrorMessage : public Messag
 
 // The format for graphics data which will be sent to the middleman process.
 // This needs to match the format expected for canvas image data, to avoid
 // transforming the data before rendering it in the middleman process.
 static const gfx::SurfaceFormat gSurfaceFormat = gfx::SurfaceFormat::R8G8B8X8;
 
 struct PaintMessage : public Message
 {
+  // Checkpoint whose state is being painted.
+  uint32_t mCheckpointId;
+
   uint32_t mWidth;
   uint32_t mHeight;
 
-  PaintMessage(uint32_t aWidth, uint32_t aHeight)
+  PaintMessage(uint32_t aCheckpointId, uint32_t aWidth, uint32_t aHeight)
     : Message(MessageType::Paint, sizeof(*this))
+    , mCheckpointId(aCheckpointId)
     , mWidth(aWidth)
     , mHeight(aHeight)
   {}
 };
 
 struct HitCheckpointMessage : public Message
 {
   uint32_t mCheckpointId;
--- a/toolkit/recordreplay/ipc/ChildIPC.cpp
+++ b/toolkit/recordreplay/ipc/ChildIPC.cpp
@@ -418,42 +418,56 @@ SetVsyncObserver(VsyncObserver* aObserve
 static void
 NotifyVsyncObserver()
 {
   if (gVsyncObserver) {
     gVsyncObserver->NotifyVsync(TimeStamp::Now());
   }
 }
 
-void
+// Whether an update has been sent to the compositor for a normal paint, and we
+// haven't reached PaintFromMainThread yet. This is used to preserve the
+// invariant that there can be at most one paint performed between two
+// checkpoints, other than repaints triggered by the debugger.
+static bool gHasActivePaint;
+
+bool
 OnVsync()
 {
   // In the repainting stress mode, we create a new checkpoint on every vsync
   // message received from the UI process. When we notify the parent about the
   // new checkpoint it will trigger a repaint to make sure that all layout and
   // painting activity can occur when diverged from the recording.
   if (parent::InRepaintStressMode()) {
     CreateCheckpoint();
   }
+
+  // After a paint starts, ignore incoming vsyncs until the paint completes.
+  return !gHasActivePaint;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Painting
 ///////////////////////////////////////////////////////////////////////////////
 
-// Graphics memory is only written on the compositor thread and read on the
-// main thread and by the middleman. The gNumPendingPaints counter is used to
-// synchronize access, so that data is not read until the paint has completed.
-static Maybe<PaintMessage> gPaintMessage;
-static size_t gNumPendingPaints;
-
-// Target buffer for the draw target created by the child process widget.
+// Target buffer for the draw target created by the child process widget, which
+// the compositor thread writes to.
 static void* gDrawTargetBuffer;
 static size_t gDrawTargetBufferSize;
 
+// Dimensions of the last paint which the compositor performed.
+static size_t gPaintWidth, gPaintHeight;
+
+// How many updates have been sent to the compositor thread and haven't been
+// processed yet. This can briefly become negative if the main thread sends an
+// update and the compositor processes it before the main thread reaches
+// NotifyPaintStart. Outside of this window, the compositor can only write to
+// gDrawTargetBuffer or update gPaintWidth/gPaintHeight if this is non-zero.
+static Atomic<int32_t, SequentiallyConsistent, Behavior::DontPreserve> gNumPendingPaints;
+
 // ID of the compositor thread.
 static Atomic<size_t, SequentiallyConsistent, Behavior::DontPreserve> gCompositorThreadId;
 
 already_AddRefed<gfx::DrawTarget>
 DrawTargetForRemoteDrawing(LayoutDeviceIntSize aSize)
 {
   MOZ_RELEASE_ASSERT(!NS_IsMainThread());
 
@@ -464,17 +478,18 @@ DrawTargetForRemoteDrawing(LayoutDeviceI
   } else {
     gCompositorThreadId = threadId;
   }
 
   if (aSize.IsEmpty()) {
     return nullptr;
   }
 
-  gPaintMessage = Some(PaintMessage(aSize.width, aSize.height));
+  gPaintWidth = aSize.width;
+  gPaintHeight = aSize.height;
 
   gfx::IntSize size(aSize.width, aSize.height);
   size_t bufferSize = layers::ImageDataSerializer::ComputeRGBBufferSize(size, gSurfaceFormat);
   MOZ_RELEASE_ASSERT(bufferSize <= parent::GraphicsMemorySize);
 
   if (bufferSize != gDrawTargetBufferSize) {
     free(gDrawTargetBuffer);
     gDrawTargetBuffer = malloc(bufferSize);
@@ -488,59 +503,58 @@ DrawTargetForRemoteDrawing(LayoutDeviceI
                                           /* aUninitialized = */ true);
   if (!drawTarget) {
     MOZ_CRASH();
   }
 
   return drawTarget.forget();
 }
 
-static void
-FinishInProgressPaint(MonitorAutoLock&)
-{
-  while (gNumPendingPaints) {
-    gMonitor->Wait();
-  }
-}
-
 void
 NotifyPaintStart()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  MonitorAutoLock lock(*gMonitor);
+  // A new paint cannot be triggered until the last one finishes and has been
+  // sent to the middleman.
+  MOZ_RELEASE_ASSERT(HasDivergedFromRecording() || !gHasActivePaint);
+
   gNumPendingPaints++;
+  gHasActivePaint = true;
+
+  CreateCheckpoint();
 }
 
 static void
 PaintFromMainThread()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
-  // If another paint started before we got here, finish it first.
-  {
-    MonitorAutoLock lock(*gMonitor);
-    FinishInProgressPaint(lock);
-  }
+  // There cannot not be any other in flight paints.
+  MOZ_RELEASE_ASSERT(!gNumPendingPaints);
 
-  if (IsActiveChild() && gPaintMessage.isSome()) {
+  // Clear the active flag now that we have completed the paint.
+  MOZ_RELEASE_ASSERT(gHasActivePaint);
+  gHasActivePaint = false;
+
+  if (IsActiveChild() && gDrawTargetBuffer) {
     memcpy(gGraphicsShmem, gDrawTargetBuffer, gDrawTargetBufferSize);
-    gChannel->SendMessage(gPaintMessage.ref());
+    gChannel->SendMessage(PaintMessage(navigation::LastNormalCheckpoint(),
+                                       gPaintWidth, gPaintHeight));
   }
 }
 
 void
 NotifyPaintComplete()
 {
   MOZ_RELEASE_ASSERT(Thread::Current()->Id() == gCompositorThreadId);
 
   // Notify the main thread in case it is waiting for this paint to complete.
   {
     MonitorAutoLock lock(*gMonitor);
-    MOZ_RELEASE_ASSERT(gNumPendingPaints);
     if (--gNumPendingPaints == 0) {
       gMonitor->Notify();
     }
   }
 
   // Notify the middleman about the completed paint from the main thread.
   NS_DispatchToMainThread(NewRunnableFunction("PaintFromMainThread", PaintFromMainThread));
 }
@@ -553,50 +567,56 @@ Repaint(size_t* aWidth, size_t* aHeight)
 
   // Don't try to repaint if the first normal paint hasn't occurred yet.
   if (!gCompositorThreadId) {
     *aWidth = 0;
     *aHeight = 0;
     return;
   }
 
-  // Create an artifical vsync to see if graphics have changed since the last
-  // paint and a new paint is needed.
-  NotifyVsyncObserver();
-
-  {
-    MonitorAutoLock lock(*gMonitor);
+  // Ignore the request to repaint if the compositor thread has already
+  // diverged from the recording. In this case we have already done a repaint
+  // and the last graphics we sent will still be correct.
+  Thread* compositorThread = Thread::GetById(gCompositorThreadId);
+  if (!compositorThread->WillDivergeFromRecordingSoon()) {
+    // Create an artifical vsync to see if graphics have changed since the last
+    // paint and a new paint is needed.
+    NotifyVsyncObserver();
 
     if (gNumPendingPaints) {
       // Allow the compositor to diverge from the recording so it can perform
-      // the paint we just triggered, or any in flight paint that that existed
-      // at the point we are paused at.
+      // any paint we just triggered, or finish any in flight paint that that
+      // existed at the point we are paused at.
       Thread::GetById(gCompositorThreadId)->SetShouldDivergeFromRecording();
 
-      FinishInProgressPaint(lock);
+      // Wait for the compositor to finish all in flight paints, including any
+      // one we just triggered.
+      MonitorAutoLock lock(*gMonitor);
+      while (gNumPendingPaints) {
+        gMonitor->Wait();
+      }
     }
   }
 
-  if (gPaintMessage.isSome()) {
+  if (gDrawTargetBuffer) {
     memcpy(gGraphicsShmem, gDrawTargetBuffer, gDrawTargetBufferSize);
-    *aWidth = gPaintMessage.ref().mWidth;
-    *aHeight = gPaintMessage.ref().mHeight;
+    *aWidth = gPaintWidth;
+    *aHeight = gPaintHeight;
   } else {
     *aWidth = 0;
     *aHeight = 0;
   }
 }
 
 static bool
 CompositorCanPerformMiddlemanCalls()
 {
   // After repainting finishes the compositor is not allowed to send call
   // requests to the middleman anymore.
-  MonitorAutoLock lock(*gMonitor);
-  return gNumPendingPaints != 0;
+  return !!gNumPendingPaints;
 }
 
 bool
 SuppressMessageAfterDiverge(IPC::Message* aMsg)
 {
   MOZ_RELEASE_ASSERT(HasDivergedFromRecording());
 
   // Only messages necessary for compositing can be sent after the sending
@@ -616,17 +636,17 @@ SuppressMessageAfterDiverge(IPC::Message
 
   return true;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Checkpoint Messages
 ///////////////////////////////////////////////////////////////////////////////
 
-// When recording, the time when the last HitCheckpoint message was sent.
+// The time when the last HitCheckpoint message was sent.
 static double gLastCheckpointTime;
 
 // When recording and we are idle, the time when we became idle.
 static double gIdleTimeStart;
 
 void
 BeginIdleTime()
 {
--- a/toolkit/recordreplay/ipc/ChildInternal.h
+++ b/toolkit/recordreplay/ipc/ChildInternal.h
@@ -71,16 +71,19 @@ void Repaint(size_t* aWidth, size_t* aHe
 // Called when running forward, immediately before hitting a normal or
 // temporary checkpoint.
 void BeforeCheckpoint();
 
 // Called immediately after hitting a normal or temporary checkpoint, either
 // when running forward or immediately after rewinding.
 void AfterCheckpoint(const CheckpointId& aCheckpoint);
 
+// Get the ID of the last normal checkpoint.
+size_t LastNormalCheckpoint();
+
 } // namespace navigation
 
 namespace child {
 
 // IPC activity that can be triggered by navigation.
 void RespondToRequest(const js::CharBuffer& aBuffer);
 void HitCheckpoint(size_t aId, bool aRecordingEndpoint);
 void HitBreakpoint(bool aRecordingEndpoint, const uint32_t* aBreakpoints, size_t aNumBreakpoints);
--- a/toolkit/recordreplay/ipc/ChildNavigation.cpp
+++ b/toolkit/recordreplay/ipc/ChildNavigation.cpp
@@ -1106,16 +1106,22 @@ void
 AfterCheckpoint(const CheckpointId& aCheckpoint)
 {
   AutoDisallowThreadEvents disallow;
 
   MOZ_RELEASE_ASSERT(IsRecordingOrReplaying());
   gNavigation->AfterCheckpoint(aCheckpoint);
 }
 
+size_t
+LastNormalCheckpoint()
+{
+  return gNavigation->LastCheckpoint().mNormal;
+}
+
 void
 DebuggerRequest(js::CharBuffer* aRequestBuffer)
 {
   gNavigation->HandleDebuggerRequest(aRequestBuffer);
 }
 
 void
 SetBreakpoint(size_t aId, const BreakpointPosition& aPosition)
--- a/toolkit/recordreplay/ipc/DisabledIPC.cpp
+++ b/toolkit/recordreplay/ipc/DisabledIPC.cpp
@@ -46,17 +46,17 @@ void CreateCheckpoint()
 }
 
 void
 SetVsyncObserver(VsyncObserver* aObserver)
 {
   MOZ_CRASH();
 }
 
-void
+bool
 OnVsync()
 {
   MOZ_CRASH();
 }
 
 void
 NotifyPaintStart()
 {
--- a/toolkit/recordreplay/ipc/JSControl.cpp
+++ b/toolkit/recordreplay/ipc/JSControl.cpp
@@ -409,17 +409,17 @@ Middleman_HadRepaint(JSContext* aCx, uns
   if (!args.get(0).isNumber() || !args.get(1).isNumber()) {
     JS_ReportErrorASCII(aCx, "Bad width/height");
     return false;
   }
 
   size_t width = args.get(0).toNumber();
   size_t height = args.get(1).toNumber();
 
-  PaintMessage message(width, height);
+  PaintMessage message(CheckpointId::Invalid, width, height);
   parent::UpdateGraphicsInUIProcess(&message);
 
   args.rval().setUndefined();
   return true;
 }
 
 static bool
 Middleman_HadRepaintFailure(JSContext* aCx, unsigned aArgc, Value* aVp)