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

Texture update strategies #2147

Merged
merged 2 commits into from Dec 4, 2017

Conversation

@kvark
Copy link
Member

commented Dec 1, 2017

Fixes #2110, hopefully: still to be benchmarked on Windows.
Provide the settings for Gecko to fix ^^

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12fecf240aa3440764cd64205b17bfcfe7eb252c

r? @glennw


This change is Reviewable

@glennw

This comment has been minimized.

Copy link
Member

commented Dec 1, 2017

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@glennw glennw self-requested a review Dec 1, 2017
@glennw
glennw approved these changes Dec 1, 2017
Copy link
Member

left a comment

Looks sane, nice one!

@kvark

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2017

Oh, forgot to mention also that the PR optimizes the GPU cache PBO transfers by allocating a buffer once instead of orphaning it multiple times.

@kvark kvark force-pushed the kvark:pbo-style branch from 83ee20d to 01cd32a Dec 1, 2017
@kvark

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2017

The try push contains quite a few failures, many related to text, so it might not be this PR. Investigating...

@glennw

This comment has been minimized.

Copy link
Member

commented Dec 1, 2017

@kvark The current wr-future bug contains a patch from @lsalzman which fixes all the font failures when trying on WR master. I hit this yesterday :)

@kvark

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2017

@glennw thanks!
Latest push with Lee's fix in it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=981a2e19c39d40f63ebe6c73fc65a6d5534ecb99

I'd say that the PR is good to go if the tests are green. If, somehow, the UploadMode::Immediate default on Windows appears to be slower, we can just select UploadMode::PixelBuffer(Stream) in the RendererOptions for Gecko/Servo until it's sorted out.

@jrmuizel

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2017

This commit may regress the CSS bouncing blend circles on AMD Windows. I need to still confirm.

@glennw

This comment has been minimized.

Copy link
Member

commented Dec 2, 2017

@jrmuizel Do you mean with native GL or ANGLE?

@kvark kvark force-pushed the kvark:pbo-style branch from 01cd32a to 61f59d8 Dec 2, 2017
@jrmuizel

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2017

This was with ANGLE

@kvark

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2017

I found the bug in the code, fixed it, and also switched the PBO upload method on Windows to the same as it is now. So the only functional change would be the bulk upload of CPU cache data.
Also, try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12fecf240aa3440764cd64205b17bfcfe7eb252c
I think it's safe to land, but more benchmarking is pending.

@kvark kvark changed the title [WIP] Texture update strategies Texture update strategies Dec 3, 2017
@kvark

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2017

Benchmarking MotionMark results on Windows/AMD/Angle

Experiment steps:

  • set up the settings (reading from a file)
  • boot up firefox in full screen
  • jump to the bouncing circles test with ramping complexity
  • test it 3 times in a row

Scores

  • Immediate: 192, 181, 168
  • PixelBuffer(Static): 156, 176, 168
  • PixelBuffer(Dynamic): 145, 142, 149
  • PixelBuffer(Stream): 153, 135, 127

Analysis

Good thing is - the scores support our expectations about what should be fast. Both Dynamic and Stream PBO uploads are slow, with the current default arguably being the worst (on this configuration!). Both Static PBO and Immediate are quite fast.

I'm going to benchmark pure GL on the main platforms as well. Here is the local patch I use to make it run-time configurable:

diff --git a/gfx/webrender_bindings/src/bindings.rs b/gfx/webrender_bindings/src/bindings.rs
index 4365919..a7c0e79 100644
--- a/gfx/webrender_bindings/src/bindings.rs
+++ b/gfx/webrender_bindings/src/bindings.rs
@@ -10,7 +10,7 @@ use gleam::gl;
 use webrender::api::*;
 use webrender::{ReadPixelsFormat, Renderer, RendererOptions, ThreadListener};
 use webrender::{ExternalImage, ExternalImageHandler, ExternalImageSource};
-use webrender::DebugFlags;
+use webrender::{DebugFlags, UploadMethod, VertexUsageHint};
 use webrender::{ApiRecordingReceiver, BinaryRecorder};
 use webrender::ProgramCache;
 use thread_profiler::register_thread_with_profiler;
@@ -718,10 +718,30 @@ pub extern "C" fn wr_window_new(window_id: WrWindowId,
         Arc::clone(&(*thread_pool).0)
     };
 
+    let upload_method = {
+        use std::fs::File;
+        use std::io::Read;
+        let mut string = String::new();
+        match File::open("c:/Code/pbo-usage.txt") {
+            Ok(mut file) => {
+                file.read_to_string(&mut string).unwrap();
+                let hint = match string.as_str() {
+                    "static" => VertexUsageHint::Static,
+                    "dynamic" => VertexUsageHint::Dynamic,
+                    "stream" => VertexUsageHint::Stream,
+                    _ => panic!(format!("Unknown usage {}", string)),
+                };
+                UploadMethod::PixelBuffer(hint)
+            },
+            Err(_) => UploadMethod::Immediate,
+        }
+    };
+
     let opts = RendererOptions {
         enable_aa: true,
         enable_subpixel_aa: true,
-        recorder: recorder,
+        recorder,
+        upload_method,
         blob_image_renderer: Some(Box::new(Moz2dImageRenderer::new(workers.clone()))),
         workers: Some(workers.clone()),
         thread_listener: Some(Box::new(GeckoProfilerThreadListener::new())),
@kvark

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2017

Windows/GL/Native scores

  • Immediate: 135, 149, 143
  • Static: 136, 143, 123
  • Dynamic: 161, 143, 145
  • Stream: 143, 141, 131

Interestingly, Dynamic works best for this case.

@kvark

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2017

Testing on my Linux laptop shows the MM scores all over the place. I think we should proceed and then have a bit of logic in Gecko to select the proper upload style (e.g. Immedaite on Angle, Dynamic on pure GL). Any concerns left about this PR?

@glennw

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

No concerns here, r=me.

@kvark

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2017

We'll keep messing with upload/Angle performance, trying out quite a few optimization paths. No need to hold this one longer.

@bors-servo r=glennw

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

📌 Commit 61f59d8 has been approved by glennw

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

⌛️ Testing commit 61f59d8 with merge c966fdb...

bors-servo added a commit that referenced this pull request Dec 4, 2017
Texture update strategies

~~Fixes  #2110, hopefully: still to be benchmarked on Windows.~~
Provide the settings for Gecko to fix ^^

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12fecf240aa3440764cd64205b17bfcfe7eb252c

r? @glennw

<!-- 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/2147)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2017

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

@bors-servo bors-servo merged commit 61f59d8 into servo:master Dec 4, 2017
4 of 5 checks passed
4 of 5 checks passed
code-review/reviewable 5 files left (glennw)
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
@kvark kvark deleted the kvark:pbo-style branch Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.