Better Hololens loop #23904

paulrouget · 2019-08-01T00:03:11Z

Depends on #23900

Fix #23823

r? @gterzian

This change is

paulrouget · 2019-08-01T00:04:24Z

@gterzian ignore the clang change and the task change.

gterzian

I like the new task() part!

Couple of questions left...

gterzian · 2019-08-01T04:29:52Z

support/hololens/ServoApp/BrowserPage.cpp

-  HANDLE hEvent = ::OpenEventA(EVENT_ALL_ACCESS, FALSE, sWakeupEvent);
-  ::SetEvent(hEvent);
+  EnterCriticalSection(&mGLLock);
+  mPendingWakeup = true;


How about just doing RunOnGLThread([=] { });, so that the flag can be removed?

Later it could also be a hook to provide a specific event to WakeUp(), where the match would happen here and the "right" task would be added to the queue?

If you use RunOnGLThread([=] { }), you don't need to use the critical section and wake stuff here(even though I think they are actually re-entrant so it wouldn't deadlock, but still...)

That what I was initially doing. Just felt like capturing the closure context on WakeUp was an unnecessary overhead. But it's probably minimal. For the sake of simplicity I might get rid of mPendingWakeup.

gterzian · 2019-08-01T04:29:52Z

ports/libsimpleservo/api/src/lib.rs

@@ -488,7 +488,8 @@ impl ServoGlue {
                            .host_callbacks
                            .on_allow_navigation(url.to_string());
                        let window_event = WindowEvent::AllowNavigationResponse(pipeline_id, data);
-                        let _ = self.process_event(window_event);
+                        self.events.push(window_event);


Perhaps add a FIXME here to say that this would ideally go through host_callbacks?

What do you mean go through host_callbacks?

If we wanted to actually ask the user for permission, it would have to go through there, right?

Isn't that what's happening two lines earlier?

gterzian · 2019-08-01T04:29:52Z

support/hololens/ServoApp/BrowserPage.cpp

-  ::SetEvent(hEvent);
+  EnterCriticalSection(&mGLLock);
+  mLooping = false;
+  LeaveCriticalSection(&mGLLock);


Here you might also need to add a WakeConditionVariable(&mGLCondVar);, to make sure the loop wakes-up and sees the change.

That should not be necessary as we know that at the point that we are not waiting on the variable.

I'm not sure I understand the flow then.

If mTasks.size() == 0 && !mAnimating && mLooping, the loop might be waiting on the condvar. In that case it won't see the change of mLooping = false unless woken-up.

And RequestShutdown doesn't seem to do anything to ensure it is not waiting by the time OnShutdownComplete is called.

Since RequestShutdown is called at https://github.com/servo/servo/pull/23904/files#diff-9f1e08fa0c4bb110f99fabbfeb609314R40, which is when mLooping, so then when OnShutdownComplete is called by Servo, mLooping would still be true, which means unless there are events or it's animating, it could be waiting, then here it is to false, and for the loop to stop it would need wake-up and see the change to !mLooping?

the loop might be waiting on the condvar

No because at that point we are running code in the compositor thread. OnShutdownComplete is called from perform_update. It's impossible to be waiting on the condvar at that point.

so after OnShutdownComplete is called, perform_udpate finishes, we get back to the beginning of the loop, mLooping is checked. And we good :)

Yep I think you're right, inside mServo->PerformUpdates() it's set to false, and then you go back to the top and you don't wait, going straight to break.

looks good.

gterzian · 2019-08-01T04:29:52Z

support/hololens/ServoApp/BrowserPage.cpp

-      ::WaitForSingleObject(hEvent, INFINITE);
-      StopRenderLoop();
+      RunOnGLThread([=] { mServo->RequestShutdown(); });
+      mLoopTask->wait();


Is this basically going to wait on cancel_current_task() to be called from inside the loop?

Since that will eventually happen as a result of the OnShutdownComplete call.

And OnShutdownComplete will not signal back here, so if this here will be notified once the loop stops, it looks right, otherwise you might want to keep the sShutdownEvent, and dispatch it when the loop stops.

Is this basically going to wait on cancel_current_task() to be called from inside the loop?

Yes.

sShutdownEvent is not necessary. Wait() will block until the loop stops, and will stop only if OnShutdownComplete is called.


        Clarify hololens loop


        Add clang-format file

gterzian · 2019-08-02T06:43:52Z

support/hololens/ServoApp/BrowserPage.cpp

-  ::SetEvent(hEvent);
+  EnterCriticalSection(&mGLLock);
+  mLooping = false;
+  LeaveCriticalSection(&mGLLock);


I'm not sure I understand the flow then.

If mTasks.size() == 0 && !mAnimating && mLooping, the loop might be waiting on the condvar. In that case it won't see the change of mLooping = false unless woken-up.

And RequestShutdown doesn't seem to do anything to ensure it is not waiting by the time OnShutdownComplete is called.

gterzian · 2019-08-02T06:43:52Z

support/hololens/ServoApp/BrowserPage.cpp

-      ::WaitForSingleObject(hEvent, INFINITE);
-      StopRenderLoop();
+      RunOnGLThread([=] { mServo->RequestShutdown(); });
+      mLoopTask->wait();


gterzian · 2019-08-02T06:44:56Z

Also one comment at #23904 (comment) (not sure why some became part of a new review and others not...)

paulrouget · 2019-08-05T05:50:12Z

@gterzian I think this can land unless I got something wrong about the OnShutdownComplete bit.

gterzian · 2019-08-05T06:00:32Z

support/hololens/ServoApp/BrowserPage.cpp

-  ::SetEvent(hEvent);
+  EnterCriticalSection(&mGLLock);
+  mLooping = false;
+  LeaveCriticalSection(&mGLLock);


Yep I think you're right, inside mServo->PerformUpdates() it's set to false, and then you go back to the top and you don't wait, going straight to break.

looks good.

paulrouget · 2019-08-05T06:16:24Z

@bors-servo r=gterzian

bors-servo · 2019-08-05T06:16:25Z

📌 Commit 20c7692 has been approved by gterzian

bors-servo · 2019-08-05T14:11:42Z

⌛ Testing commit 20c7692 with merge e76c880...


        Auto merge of #23904 - paulrouget:loop2, r=gterzian

@gterzian

Better Hololens loop Depends on #23900 Fix #23823 r? @gterzian  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23904)

bors-servo · 2019-08-05T14:52:38Z

💔 Test failed - linux-rel-wpt

CYBAI · 2019-08-05T14:58:26Z

@bors-servo retry

#23915

bors-servo · 2019-08-05T17:22:49Z

💣 Failed to start rebuilding: Unknown error

bors-servo · 2019-08-05T17:22:53Z

⌛ Testing commit 20c7692 with merge a1a07f4...


        Auto merge of #23904 - paulrouget:loop2, r=gterzian

@gterzian

Better Hololens loop Depends on #23900 Fix #23823 r? @gterzian  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23904)

bors-servo · 2019-08-05T18:08:52Z

💔 Test failed - linux-rel-wpt

paulrouget · 2019-08-07T10:21:32Z

@bors-servo retry

bors-servo · 2019-08-07T10:21:38Z

⌛ Testing commit 20c7692 with merge 371f514...


        Auto merge of #23904 - paulrouget:loop2, r=gterzian

@gterzian

Better Hololens loop Depends on #23900 Fix #23823 r? @gterzian  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23904)

bors-servo · 2019-08-07T13:09:54Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: gterzian
Pushing 371f514 to master...

highfive assigned gterzian Aug 1, 2019

highfive added the S-awaiting-review label Aug 1, 2019

gterzian suggested changes Aug 1, 2019

View changes

paulrouget added 2 commits Aug 1, 2019

Clarify hololens loop

b7667ff

Add clang-format file

Loading status checks…

20c7692

paulrouget changed the title ~~[wip] Better Hololens loop~~ Better Hololens loop Aug 1, 2019

paulrouget force-pushed the paulrouget:loop2 branch from 70a9658 to 20c7692 Aug 1, 2019

gterzian suggested changes Aug 2, 2019

View changes

gterzian approved these changes Aug 5, 2019

View changes

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 5, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 5, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 5, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 5, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 7, 2019

bors-servo merged commit 20c7692 into servo:master Aug 7, 2019
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Aug 7, 2019

servo / servo

Better Hololens loop #23904

Better Hololens loop #23904

paulrouget commented Aug 1, 2019 •

edited by SimonSapin

paulrouget commented Aug 1, 2019

gterzian left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian commented Aug 2, 2019

paulrouget commented Aug 5, 2019

This comment has been minimized.

paulrouget commented Aug 5, 2019

bors-servo commented Aug 5, 2019

bors-servo commented Aug 5, 2019

bors-servo commented Aug 5, 2019

CYBAI commented Aug 5, 2019

bors-servo commented Aug 5, 2019

bors-servo commented Aug 5, 2019

bors-servo commented Aug 5, 2019

paulrouget commented Aug 7, 2019

bors-servo commented Aug 7, 2019

bors-servo commented Aug 7, 2019

servo / servo

Join GitHub today

Better Hololens loop #23904

Better Hololens loop #23904

Conversation

paulrouget commented Aug 1, 2019 • edited by SimonSapin

paulrouget commented Aug 1, 2019

gterzian left a comment

This comment has been minimized.

gterzian Aug 1, 2019 • edited

This comment has been minimized.

gterzian Aug 1, 2019 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Aug 1, 2019 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Aug 2, 2019 • edited

This comment has been minimized.

gterzian Aug 2, 2019 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Aug 1, 2019 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Aug 2, 2019 • edited

This comment has been minimized.

gterzian commented Aug 2, 2019

paulrouget commented Aug 5, 2019

This comment has been minimized.

paulrouget commented Aug 5, 2019

bors-servo commented Aug 5, 2019

bors-servo commented Aug 5, 2019

bors-servo commented Aug 5, 2019

CYBAI commented Aug 5, 2019

bors-servo commented Aug 5, 2019

bors-servo commented Aug 5, 2019

bors-servo commented Aug 5, 2019

paulrouget commented Aug 7, 2019

bors-servo commented Aug 7, 2019

bors-servo commented Aug 7, 2019

paulrouget commented Aug 1, 2019 •

edited by SimonSapin

gterzian Aug 1, 2019 •

edited

gterzian Aug 1, 2019 •

edited

gterzian Aug 1, 2019 •

edited

gterzian Aug 2, 2019 •

edited

gterzian Aug 2, 2019 •

edited

gterzian Aug 1, 2019 •

edited

gterzian Aug 2, 2019 •

edited