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

Instanced attributes #615

Merged
merged 1 commit into from Dec 13, 2016
Merged

Instanced attributes #615

merged 1 commit into from Dec 13, 2016

Conversation

@kvark
Copy link
Member

kvark commented Dec 2, 2016

Closes #457

Performance-wise, I wasn't able to register a noticeable difference. Tested on https://github.com/servo/servo, full screen after the second page down, on Mesa DRI Intel(R) HD Graphics 5500 (Broadwell GT2) with resolution 2560x1440.

With the change, I got 4.5 ms mean GPU time in the first test, and 4.3 ms time in the second.
Without the change, I got 4.4 ms mean GPU time in the only test.

I suppose the difference is not visible since we are far from being VS-bound.

Note: this does not replace the optimization of having one large buffer in #456. Instanced attributes will benefit from it in the same way as UBOs.


This change is Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Dec 2, 2016

The latest upstream changes (presumably #614) made this pull request unmergeable. Please resolve the merge conflicts.

@kvark kvark force-pushed the kvark:instance branch from 5b55062 to a223350 Dec 2, 2016
@kvark kvark changed the title Instanced attributes [WIP] Instanced attributes Dec 3, 2016
@glennw
Copy link
Member

glennw commented Dec 9, 2016

@kvark Is this still WIP or ready for review?

@bors-servo
Copy link
Contributor

bors-servo commented Dec 9, 2016

The latest upstream changes (presumably #623) made this pull request unmergeable. Please resolve the merge conflicts.

@kvark
Copy link
Member Author

kvark commented Dec 9, 2016

@glennw
Copy link
Member

glennw commented Dec 9, 2016

@kvark OK - would you prefer we land this after review, or wait until the UBO removal is done?

@kvark
Copy link
Member Author

kvark commented Dec 9, 2016

Let me remove the UBOs completely in this PR. Meanwhile, @jamienicol is going to check for the HW extensions support on GLES w.r.t. the instanced attributes.

@kvark kvark changed the title [WIP] Instanced attributes Instanced attributes Dec 12, 2016
@kvark kvark force-pushed the kvark:instance branch from 3980378 to 740b90d Dec 12, 2016
@kvark
Copy link
Member Author

kvark commented Dec 13, 2016

The PR is ready for review now.
I've made more perf tests on the servo page and barely noticed any changes. The GPU time is about 3.5ms, the Compositor time jumps a bit, with the minimum bound about 0.80ms for the original version and 0.75ms for the new one. This is well within the measurement error threshold.

@glennw
Copy link
Member

glennw commented Dec 13, 2016

Reviewed 23 of 24 files at r1.
Review status: 23 of 24 files reviewed at latest revision, 1 unresolved discussion.


webrender/src/device.rs, line 190 at r1 (raw file):

                                            gl::INT,
                                            instance_stride,
                                            24);

Can we accumulate a variable here? Otherwise it'll be really easy to add an item to the array above and forget to update this value.


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Dec 13, 2016

@kvark Looks good, just one small issue to address.

@kvark kvark force-pushed the kvark:instance branch from 740b90d to 6e10fd2 Dec 13, 2016
@kvark
Copy link
Member Author

kvark commented Dec 13, 2016

@glennw thanks for having a look. The offset is now calculated, and I agree this is better.

@glennw
Copy link
Member

glennw commented Dec 13, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Dec 13, 2016

📌 Commit 6e10fd2 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Dec 13, 2016

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Dec 13, 2016

The latest upstream changes (presumably #630) made this pull request unmergeable. Please resolve the merge conflicts.

@kvark kvark force-pushed the kvark:instance branch from 6e10fd2 to 68f63db Dec 13, 2016
@kvark
Copy link
Member Author

kvark commented Dec 13, 2016

Rebased now.

@glennw
Copy link
Member

glennw commented Dec 13, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Dec 13, 2016

📌 Commit 68f63db has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Dec 13, 2016

Testing commit 68f63db with merge 482b59b...

bors-servo added a commit that referenced this pull request Dec 13, 2016
Instanced attributes

Closes #457

Performance-wise, I wasn't able to register a noticeable difference. Tested on https://github.com/servo/servo, full screen after the second page down, on `Mesa DRI Intel(R) HD Graphics 5500 (Broadwell GT2)` with resolution 2560x1440.

With the change, I got 4.5 ms mean GPU time in the first test, and 4.3 ms time in the second.
Without the change, I got 4.4 ms mean GPU time in the only test.

I suppose the difference is not visible since we are far from being VS-bound.

Note: this does not replace the optimization of having one large buffer in #456. Instanced attributes will benefit from it in the same way as UBOs.

<!-- 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/615)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 13, 2016

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit 68f63db into servo:master Dec 13, 2016
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 5 files, 1 discussion left (glennw, kvark)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@kvark kvark deleted the kvark:instance branch Dec 14, 2016
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

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