New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define that sync objects do not become signaled until control returns to the event loop. #2598

Merged
merged 1 commit into from Feb 8, 2018

Conversation

Projects
None yet
3 participants
@kenrussell
Copy link
Contributor

kenrussell commented Feb 7, 2018

Follow the same semantics for sync objects as were previously used for
queries. Add a test for the new behavior.

@kenrussell

This comment has been minimized.

Copy link
Contributor

kenrussell commented Feb 7, 2018

@jdashg please review.

@zhenyao @kainino0x @jdarpinian FYI.

@kainino0x
Copy link
Contributor

kainino0x left a comment

The spec text LGTM. We may want to announce on public_webgl once we update implementations, too.

@kainino0x

This comment has been minimized.

Copy link
Contributor

kainino0x commented Feb 7, 2018

Tests LGTM as well.

@jdashg

jdashg approved these changes Feb 8, 2018

// Verify as best as possible that the implementation doesn't allow a sync
// object to become signaled in the same frame, by spin-looping for some time.
var startTime = Date.now();
while (Date.now() - startTime < 2000) {

This comment has been minimized.

@jdashg

jdashg Feb 8, 2018

Contributor

300ms-1000ms is fine. 2000 seems unnecessarily high.

This comment has been minimized.

@kenrussell

kenrussell Feb 8, 2018

Contributor

Changed to 1000 ms.

if (startTimeOfFinish == 0) {
startTimeOfFinish = Date.now();
}
if (Date.now() - startTimeOfFinish > 2000) {

This comment has been minimized.

@jdashg

jdashg Feb 8, 2018

Contributor

2000 used without being a named constant again. Please name and define one.

This comment has been minimized.

@kenrussell

kenrussell Feb 8, 2018

Contributor

Thanks for your feedback. Named and defined one.

@jdashg

This comment has been minimized.

Copy link
Contributor

jdashg commented Feb 8, 2018

Ok, this works for me. The title here is inaccurate though, since this matches the query behavior that the app must return to the event loop, not necessarily the next frame. It's worth correcting so the changelog shows what we actually did.

Define that sync objects do not become signaled until control returns
to the event loop.

Follow the same semantics for sync objects as were previously used for
queries. Add a test for the new behavior.

@kenrussell kenrussell force-pushed the kenrussell:syncs-not-available-in-current-frame branch from 3719480 to 126cbdc Feb 8, 2018

@kenrussell

This comment has been minimized.

Copy link
Contributor

kenrussell commented Feb 8, 2018

Thanks @jdashg for your review. I rebased, squashed and edited the commit message to be accurate. Will update the pull request title here too. Merging.

@kenrussell kenrussell changed the title Define that sync objects do not become signaled in the current frame. Define that sync objects do not become signaled until control returns to the event loop. Feb 8, 2018

@kenrussell kenrussell merged commit 5edec04 into KhronosGroup:master Feb 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kenrussell kenrussell deleted the kenrussell:syncs-not-available-in-current-frame branch Feb 8, 2018

ardalanamini pushed a commit to siliconjs/chromium that referenced this pull request Feb 8, 2018

Change sync objects to not be available in the current frame.
Follows similar restrictions for query objects and implements
KhronosGroup/WebGL#2598 .

Disable EXT_disjoint_timer_query at the WebGL level.

Bug: 808744
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I178f08fd30bf252865abf2744636b4e9b3a0e677
Reviewed-on: https://chromium-review.googlesource.com/906402
Commit-Queue: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535327}

GnorTech pushed a commit to nwjs/chromium.src that referenced this pull request Feb 15, 2018

Change sync objects to not be available in the current frame.
Follows similar restrictions for query objects and implements
KhronosGroup/WebGL#2598 .

Disable EXT_disjoint_timer_query at the WebGL level.

TBR=kbr@chromium.org

(cherry picked from commit d4ff25f)

Bug: 808744
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I178f08fd30bf252865abf2744636b4e9b3a0e677
Reviewed-on: https://chromium-review.googlesource.com/906402
Commit-Queue: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#535327}
Reviewed-on: https://chromium-review.googlesource.com/917009
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#452}
Cr-Branched-From: bc084a8-refs/heads/master@{#530369}

rogerwang pushed a commit to nwjs/chromium.src that referenced this pull request Feb 22, 2018

Change sync objects to not be available in the current frame.
(Manual M65 merge-back)

Follows similar restrictions for query objects and implements
KhronosGroup/WebGL#2598 .

Disable EXT_disjoint_timer_query at the WebGL level.

TBR=dcheng@chromium.org, kainino@chromium.org, piman@chromium.org, zmo@chromium.org

Bug: 808744
Change-Id: Ibf27298392cabb87fa0222e18145682a55392997
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/919154
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#464}
Cr-Branched-From: bc084a8-refs/heads/master@{#530369}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment