Add program_binary support #2032
Conversation
|
Cargo.lock has two gl_generator version, since "servo-glutin" still uses "gl_generator 0.5.4. |
servo/glutin#129 is going to handle it. |
There was a problem hiding this comment.
Thanks @sotaroikeda for pushing this forward!
My main concern would be having a lazy static map of program binaries. If we want (and I'm pretty sure we do) to cache the binaries on disk and the load on FF start, we should better explicitly manage this structure by the WR clients.
The API could look like:
impl Renderer {
// create with a given program cache
fn new(...,
cached_programs: Option<&ProgramCache>,
) -> ... {... }
// update the program cache with new binaries, e.g. when some of the lazy loaded shader programs got activated in the mean time
fn update_program_cache(&self, &mut ProgramCache);
}
impl ProgramCache {
// empty
pub fn new() -> Self {...}
// load from disk
pub fn load(file) -> Self {..}
// save to disk
pub fn save(&self, file) {.. }
}
webrender/src/device.rs
Outdated
| ); | ||
| } else { | ||
| // Bind vertex attributes | ||
| for (i, attr) in descriptor |
There was a problem hiding this comment.
this should not be necessary:
Additionally, all vertex shader input and fragment shader output assignments that were in effect when the program was linked before saving are restored with glProgramBinary is called
webrender/src/renderer.rs
Outdated
| @@ -3791,6 +3794,11 @@ impl Renderer { | |||
| self.ps_composite.deinit(&mut self.device); | |||
| self.device.end_frame(); | |||
| } | |||
|
|
|||
| #[allow(dead_code)] | |||
|
☔ The latest upstream changes (presumably #2041) made this pull request unmergeable. Please resolve the merge conflicts. |
sotaroikeda
force-pushed
the
binary
branch
3 times, most recently
from
ec3c5b5
to
04f93e0
Compare
|
Update the pull request based on the comments. But it does not handle "load from disk" nor "save to disk" yet. But it improves performance of 2nd shader load a lot with Windows + ANGLE. |
|
Thanks @sotaroikeda !
Yes, I'm not saying that you should implement this within the PR. What I'm saying that the architecture chosen for this PR is not going to scale to what we need eventually, and likely soon. So it would be nice to have the foundation in place that would not need to be rewritten, especially if we have an idea of how it should look like. |
|
@kvark, can you clarify more about what kind of foundation are you expecting? |
I am not sure if latest pull request has an foundational problem. |
There was a problem hiding this comment.
Ah, I see now, thanks @sotaroikeda !
Could someone from the Gecko side confirm that this API would work for them?
Aside from that and a few nits I mentioned, this is good to go.
webrender/src/device.rs
Outdated
| .bind_attrib_location(pid, i as gl::GLuint, attr.name); | ||
| let mut loaded = false; | ||
|
|
||
| if self.cached_programs.is_some() { |
There was a problem hiding this comment.
given that you are accessing it right afterwards, would be simpler to do
if let Some(cached_programs) = self.cached_programs {
}
webrender/src/device.rs
Outdated
| } | ||
| } | ||
|
|
||
| if self.cached_programs.is_some() { |
There was a problem hiding this comment.
similarly, should probably start with if let Some(cached_programs) = self.cached_programs {...}
|
I updated the pull request by applying the comment and am asking @nical a feedback in Bug 1391159 |
|
In Bug 1391159, feedback of @nical was feedback+. |
|
Ok, great, thanks @sotaroikeda ! |
|
📌 Commit 35c22be has been approved by |
Add program_binary support Fixes #2005, but still WIP. I did implementation, but I am not sure if it is a way to go. <!-- 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/2032) <!-- Reviewable:end -->
|
☀️ Test successful - status-appveyor, status-travis |
|
@kvark @sotaroikeda This seems a bit odd - do we really need to introduce a lifetime parameter onto the Renderer object? |
|
It seems like this would require the Servo compositor object to introduce a lifetime parameter, which is likely to complicate the code quite a bit. I haven't looked through this PR completely - but could we instead make the program cache a trait object with a callback, or something like that? What's the reason the device holds a mutable reference to the cache? |
Ok, I am going to work as to use trait object.
It was requested by a compiler to call cached_programs.insert. |
|
OK, sounds good. Thank you! |
Remove lifetime parameter of Renderer In #2032, @glennw commented about the problem about lifetime parameter of Renderer. The lifetime parameter could cause a problem to merge to Servo. This pull request addresses it. There could be another ways to address the problem. cc @glennw @kvark <!-- 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/2064) <!-- Reviewable:end -->
Fixes #2005, but still WIP. I did implementation, but I am not sure if it is a way to go.
This change is