Closed Bug 1098701 Opened 5 years ago Closed 5 years ago

[Silk] Gtest for project Silk

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: boris, Assigned: mchang)

References

Details

Attachments

(2 files, 10 obsolete files)

3.62 KB, patch
kats
: review+
Details | Diff | Splinter Review
4.84 KB, patch
mchang
: review+
Details | Diff | Splinter Review
We have to implement the google test for our new silk framework. This makes it easy to find out the problems in our silk code.
This patch is from Bug 1048667: Implement Vsync dispatch framework. CJ implemented this test for our original framework. Silk framework is different now, so we should update it. Put this patch here for our reference.
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
Blocks: 1078160
Blocks: 1102631
Assignee: boris.chiou → mchang
Component: Performance → Graphics
Product: Firefox OS → Core
Attached patch Gtests for Silk (obsolete) — Splinter Review
Current gtest framework that uses a software vsync timer to imitate vsync. Currently only has two tests:

1) Can enable/disable vsync.
2) Can create a CompositorVsyncDispatcher and dispatch vsync events to VsyncObservers.
Attachment #8522656 - Attachment is obsolete: true
Attached patch Gtests for Silk v2 (obsolete) — Splinter Review
Updated to poll for vsync notifications in the TestVsyncObserver.
Attachment #8544884 - Attachment is obsolete: true
Attached patch Gtests for silk v3 (obsolete) — Splinter Review
Updated to implement an option to disable thread assertions like APZC. https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp#828
Attachment #8546122 - Attachment is obsolete: true
Attached patch Gtests for Silk v4 (obsolete) — Splinter Review
Gtests for silk. Rebased on master, bug fixes from disabling vsync.
Attachment #8548364 - Attachment is obsolete: true
Useful for gtests to ensure that we can add/remove a CompositorVsyncDispatcher.
Attachment #8549213 - Attachment is obsolete: true
Attachment #8551925 - Flags: review?(bugmail.mozilla)
Since gtest runs on the main thread, allow disabling thread assertions in the CompositorVsyncDispatcher. Much like APZ gtests - https://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.h?from=AsyncPanZoomController.h&case=true#1048
Attachment #8551927 - Flags: review?(bugmail.mozilla)
Create a TestVsyncObserver. It polls for 50 ms on the main thread to ensure a vsync notification occurs. This will be ok since the Vsync always occurs off the main thread.
Attachment #8551928 - Flags: review?(bugmail.mozilla)
Attached patch Part 4: Actual gtests (obsolete) — Splinter Review
The actual gtests. Two tests included for now:

1) Ensure that we can enable/disable vsync
2) Ensure that the CompositorVsyncDispatcher gets vsync notifications.

I will add RefreshTimerVsyncDispatcher gtests in a follow up bug once we get closer to enabling the refresh driver by default.
Attachment #8551929 - Flags: review?(bugmail.mozilla)
Comment on attachment 8551928 [details] [diff] [review]
Part 3: Create TestVsyncObserver and gtest Skeleton

Review of attachment 8551928 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/tests/gtest/TestVsync.cpp
@@ +45,5 @@
> +    TimeStamp start = TimeStamp::Now();
> +    for (;;)
> +    {
> +      TimeDuration duration = TimeStamp::Now() - start;
> +      if (duration.ToMilliseconds() > kVsyncTimeoutMS) {

Instead of busy-looping, use MonitorAutoLock::Wait and ::Notify
Attachment #8551928 - Flags: review?(bugmail.mozilla) → review-
All of these patches need commit messages.
Comment on attachment 8551929 [details] [diff] [review]
Part 4: Actual gtests

Review of attachment 8551929 [details] [diff] [review]:
-----------------------------------------------------------------

Combine this patch with part 3; not much point keeping them separate here.

::: gfx/tests/gtest/TestVsync.cpp
@@ +95,5 @@
> +  globalDisplay.DisableVsync();
> +  ASSERT_TRUE(!globalDisplay.IsVsyncEnabled());
> +
> +  globalDisplay.EnableVsync();
> +  ASSERT_FALSE(globalDisplay.IsVsyncEnabled());

This should be ASSERT_TRUE. If anything the other two asserts in this function could be converted to ASSERT_FALSE.

@@ +119,5 @@
> +  ASSERT_FALSE(globalDisplay.ContainsCompositorVsyncDispatcher(vsyncDispatcher));
> +
> +  nsRefPtr<TestVsyncObserver> testVsyncObserver = new TestVsyncObserver();
> +  vsyncDispatcher->SetCompositorVsyncObserver(testVsyncObserver);
> +  ASSERT_TRUE(globalDisplay.ContainsCompositorVsyncDispatcher(vsyncDispatcher));

I don't think you need to assert this; the fact that the testVsyncObserver gets a vsync notification is proof enough that this is true. And removing this assert means you can get rid of part 1. I would prefer that because it exposes a messy hook in production code just for test purposes.
Attachment #8551929 - Flags: review?(bugmail.mozilla) → feedback+
Comment on attachment 8551925 [details] [diff] [review]
Part 1: Allow querying the Display to see if a CompositorVsyncDispatcher exists.

Review of attachment 8551925 [details] [diff] [review]:
-----------------------------------------------------------------

See comment on part 4, we can do without this.
Attachment #8551925 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 8551927 [details] [diff] [review]
Part 2: Allow Disabling Thread Assertions

Review of attachment 8551927 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ +15,5 @@
>  #endif
>  
>  namespace mozilla {
> +static bool sThreadAssertionsEnabled = true;
> +void CompositorVsyncDispatcher::SetThreadAssertionsEnabled(bool aEnable)

nit: add a blank line before this function

@@ +58,5 @@
> +  if (!sThreadAssertionsEnabled) {
> +    return;
> +  }
> +
> +  MOZ_ASSERT(layers::CompositorParent::IsInCompositorThread());

I'd rather you used Compositor::AssertOnCompositorThread() here, and renamed this function to CompositorVsyncDispatcher::AssertOnCompositorThread

@@ +90,5 @@
>    bool observeVsync = aVsyncObserver != nullptr;
> +  if (NS_IsMainThread()) {
> +    // Should only occur under test conditions
> +    MOZ_ASSERT(!sThreadAssertionsEnabled);
> +    ObserveVsync(observeVsync);

I'm not a fan of this. It seems like the more tests you add the more this kind of code is going to be littered everywhere. I suggest talking to BenWa about this, he's been working on being able to write proper compositor gtests and you might be able to use his work to have less hacks here.

::: widget/VsyncDispatcher.h
@@ +46,5 @@
>    void SetCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
>    void Shutdown();
>  
> +  // Useful for gtests which only run on one thread. This will disable
> +  // thread assertions.

"This can be used to enable or disable thread assertions. This is useful for gtests because usually things run in only one thread in that environment"

@@ +53,2 @@
>  private:
> +  void AssertInCompositorThread();;

s/;;/;/
Attachment #8551927 - Flags: review?(bugmail.mozilla)
So I think I would prefer an approach where instead of doing

  if (NS_IsMainThread()) {
    // Should only occur under test conditions
    Func();
  } else {
    NS_DispatchToMainThread(..Func..)
  }

you leave the original code as-is (i.e. NS_DispatchToMainThread) and then in the gtest follow it up with calling NS_GetMainThread [1] and then run |while (thread->processNextEvent(false));| to process the pending dispatch-to-main-thread events. Give that a shot and see if it works.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp?rev=b71ccef9c674#113
[2] http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsIThread.idl?rev=7e51a10dee23#67
Attached patch Part1: Gtest Framework and Tests (obsolete) — Splinter Review
Updated with feedback from comments 10-13, 15.

1) Use notify/wait in the lock instead of polling
2) Flush notifications on main thread.
Attachment #8552125 - Flags: review?(bugmail.mozilla)
Updated with feedback from comment 14. 

> I'm not a fan of this. It seems like the more tests you add the more this 
> kind of code is going to be littered everywhere. I suggest talking to BenWa 
> about this, he's been working on being able to write proper compositor gtests 
> and you might be able to use his work to have less hacks here.

I talked with BenWa about this. Currently, his work is to spin up the actual Compositor and their backends, not the CompositorParent/Child pair which is where the CompositorVsyncObserver is. We both agreed it'd be nice to have gtests for both vsync in isolate and large internal tests.

We can do large internal gtests in a follow up bug, which will require some work to mock up both the CompositorParent/Child pair. We can keep this bug to have gtests for small vsync verification unit tests.
Attachment #8551925 - Attachment is obsolete: true
Attachment #8551927 - Attachment is obsolete: true
Attachment #8551928 - Attachment is obsolete: true
Attachment #8551929 - Attachment is obsolete: true
Attachment #8552126 - Flags: review?(bugmail.mozilla)
Comment on attachment 8552125 [details] [diff] [review]
Part1: Gtest Framework and Tests

Review of attachment 8552125 [details] [diff] [review]:
-----------------------------------------------------------------

Just to be clear, this test won't actually get exercised on B2G at all, right? It will only start getting exercised once we turn on the HardwareVsyncEnabled pref on OS X (or other desktop platforms)?

::: gfx/tests/gtest/TestVsync.cpp
@@ +43,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    MonitorAutoLock lock(mVsyncMonitor);
> +    PRIntervalTime timeout = PR_MillisecondsToInterval(kVsyncTimeoutMS);
> +    lock.Wait(timeout);

Early return before the Wait call if mDidGetVsyncNotification is already true.

@@ +46,5 @@
> +    PRIntervalTime timeout = PR_MillisecondsToInterval(kVsyncTimeoutMS);
> +    lock.Wait(timeout);
> +  }
> +
> +  bool mDidGetVsyncNotification;

Make this private and expose a getter function. Also add a comment that it is protected by mVsyncMonitor

@@ +85,5 @@
> +  rv = NS_OK;
> +  bool processed = true;
> +  while (processed && NS_SUCCEEDED(rv)) {
> +    rv = mainThread->ProcessNextEvent(false, &processed);
> +  }

you could also ASSERT_TRUE(NS_SUCCEEDED(rv)) at the bottom of the loop and take it out of the loop condition if you want. this is fine too.
Attachment #8552125 - Flags: review?(bugmail.mozilla) → review+
Attachment #8552126 - Flags: review?(bugmail.mozilla) → review+
Updated with feedback from comment 18.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> Comment on attachment 8552125 [details] [diff] [review]
> Part1: Gtest Framework and Tests
> 
> Review of attachment 8552125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just to be clear, this test won't actually get exercised on B2G at all,
> right? It will only start getting exercised once we turn on the
> HardwareVsyncEnabled pref on OS X (or other desktop platforms)?

Unfortunately :(. Still helps locally with testing though since I force enable the prefs on desktop.
Attachment #8552125 - Attachment is obsolete: true
Attachment #8552597 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/121f665885d5
https://hg.mozilla.org/mozilla-central/rev/1649a66f18e9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8552126 [details] [diff] [review]
Part 2: Allow Disabling Thread Assertions

Review of attachment 8552126 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ +62,5 @@
> +  if (!sThreadAssertionsEnabled) {
> +    return;
> +  }
> +
> +  Compositor::AssertOnCompositorThread();

layers::Compositor::AssertOnCompositorThread();
Depends on: 1125588
You need to log in before you can comment on or make changes to this bug.