Skip to content
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

GPU queries refactor #2041

Merged
merged 1 commit into from Nov 16, 2017
Merged

GPU queries refactor #2041

merged 1 commit into from Nov 16, 2017

Conversation

@kvark
Copy link
Member

kvark commented Nov 15, 2017

Removes query compile feature. Fixes #1994
Refactors the GPU queries quite a bit, including moving the logic into a dedicated module.
Ideally should be tested on Android, but we can patch it afterwards anyway.

Note: the API is asking for some type-level protection against:

  • doing anything outside of frame bounds
  • starting to sample queries in a row
    But I don't see a way to make it stronger without introducing obscure programming patterns :) So I'm leaving it with just start/finish semantics, plus a bit of RAII.

This change is Reviewable

@kvark kvark requested a review from glennw Nov 15, 2017
@glennw
Copy link
Member

glennw commented Nov 15, 2017

I didn't get to this today, sorry - will do first thing in the morning. It looks great so far from a quick scan though!

CI failures:

$ if [ $BUILD_KIND = DEBUG ]; then (cd webrender && cargo build --verbose --features profiler,query); fi

error: Package `webrender v0.53.2 (file:///home/travis/build/servo/webrender/webrender)` does not have these features: `query`

The command "if [ $BUILD_KIND = DEBUG ]; then (cd webrender && cargo build --verbose --features profiler,query); fi" exited with 101.
@glennw
Copy link
Member

glennw commented Nov 15, 2017

(We probably need to update both the travis.yml and the taskcluster config)

@glennw
Copy link
Member

glennw commented Nov 15, 2017

Reviewed 12 of 14 files at r1.
Review status: 12 of 14 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


webrender/src/profiler.rs, line 882 at r1 (raw file):

        );

        if false {

Should we remove this or comment it perhaps?


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Nov 15, 2017

(Partial review of the easy bits so far - will cover the remaining files in the morning).

@glennw
Copy link
Member

glennw commented Nov 15, 2017

Reviewed 2 of 14 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


Comments from Reviewable

@glennw
glennw approved these changes Nov 15, 2017
Copy link
Member

glennw left a comment

Looks good, thanks!

@glennw
Copy link
Member

glennw commented Nov 15, 2017

I noticed that when I toggle the query feature on and off, a small box occurs in the top left of the screen - it looks like perhaps the quad for the profile counters is still being drawn?

Not a big deal anyway - we can fix that as a follow up if you like.

So, r=me - I'll leave it to you to squash / fix that bug if you want to, or merge now :)

@kvark kvark force-pushed the kvark:query branch from ab0a383 to 19731c1 Nov 16, 2017
@kvark
Copy link
Member Author

kvark commented Nov 16, 2017

Thanks @glennw ! Fix appeared to be rather trivial: 19731c1#diff-59bdb33ab120f3af70ac6ca84fd5451fR882

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2017

📌 Commit 19731c1 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2017

Testing commit 19731c1 with merge 0f95ff0...

bors-servo added a commit that referenced this pull request Nov 16, 2017
GPU queries refactor

Removes `query` compile feature. Fixes  #1994
Refactors the GPU queries quite a bit, including moving the logic into a dedicated module.
Ideally should be tested on Android, but we can patch it afterwards anyway.

Note: the API is asking for some type-level protection against:
  - doing anything outside of frame bounds
  - starting to sample queries in a row
But I don't see a way to make it stronger without introducing obscure programming patterns :) So I'm leaving it with just start/finish semantics, plus a bit of RAII.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2041)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing 0f95ff0 to master...

@bors-servo bors-servo merged commit 19731c1 into servo:master Nov 16, 2017
4 of 5 checks passed
4 of 5 checks passed
code-review/reviewable 1 file, 1 discussion left (glennw, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@staktrace
Copy link
Contributor

staktrace commented Nov 16, 2017

@kvark This caused webrender to crash on windows, see https://bugzilla.mozilla.org/show_bug.cgi?id=1417062#c5

@kvark kvark deleted the kvark:query branch Nov 16, 2017
bors-servo added a commit that referenced this pull request Nov 16, 2017
Query GL assertion removal

Fixes #2041 (comment)
r? anyone

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2049)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.