Adding more caching to GL backend. #1221

icefoxen · Apr 5, 2017

Like #1220 except based on the right starting point. Sorry.

kvark · Apr 5, 2017

kvark reviewed Apr 5, 2017

View changes

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

+    }
+
+    fn bind_constant_buffer(&mut self, buf: &mut Vec<Command>, constant_buffer: c::pso::ConstantBufferParam<Resources>) {
+        if let Some(cb) = self.constant_buffer {


nit: could be simplified to if self.constant_buffer == Some(constant_buffer)
if it complains about the type being moved and such, you can compare by reference if self.constant_buffer.as_ref() == Some(&constant_buffer)

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

    Command::SetScissor(None),
    Command::SetDepthState(None),
    Command::SetStencilState(None, (0, 0), s::CullFace::Nothing),
    Command::SetBlendState(0, COLOR_DEFAULT),
    Command::SetBlendState(1, COLOR_DEFAULT),
    Command::SetBlendState(2, COLOR_DEFAULT),
    Command::SetBlendState(3, COLOR_DEFAULT),
-    Command::SetBlendColor([0f32; 4]),
+    Command::SetBlendColor([0f32; 4])


is that a rustfmt decision to remove the comma?

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

    }
+
+    fn bind_resource_view(&mut self, buf: &mut Vec<Command>, resource_view: c::pso::ResourceViewParam<Resources>) {
+        if let Some(rv) = self.resource_view {


similarly, this can be rewritten

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

+    }
+
+    fn bind_framebuffer(&mut self, buf: &mut Vec<Command>, access: Access, fb: FrameBuffer) {
+


extra line?

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

+        // That's actually bad because it makes it impossible
+        // to completely remove all redundant calls if the
+        // stencil is enabled;
+        // we'll be re-enabling it over and over.


kvark · Apr 5, 2017

src/backend/gl/src/command.rs

-                (_, None) => (),
+        if self.cache.current_vbs == Some(vbs) {
+            return
+        } else {


no need for else branch here

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

-                                          dst_offset_bytes as gl::types::GLintptr,
-                                          size_bytes as gl::types::GLsizeiptr));
+        self.cache.push(&mut self.buf,
+                                     Command::CopyBuffer(src,


indentation is broken here

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

-                error!("GL: unable to update the contents of a Surface({})", s),
+            NewTexture::Texture(t) => {
+                self.cache.push(&mut self.buf,
+                                             Command::UpdateTexture(t, kind, face, ptr, img))


indentation here

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

+                 count: c::VertexCount,
+                 instances: Option<command::InstanceParams>) {
+        self.cache.push(&mut self.buf,
+                                     Command::Draw(self.cache.primitive, start, count, instances));


indentation

kvark · Apr 5, 2017

src/backend/gl/src/command.rs

-            gl_index, RawOffset(offset as *const gl::types::GLvoid), count, base, instances));
+        self.cache
+            .push(&mut self.buf,
+                         Command::DrawIndexed(self.cache.primitive,


indentation

icefoxen · Apr 10, 2017

What is the policy on rustfmt? You don't appear to be using it as far as I can tell, so a lot of the indentation stuff is just emacs's auto-formatting. If there's some specific formatting guide or such I should follow that'd make life easier; I tried to follow the existing style, but only partially succeeded. ;-)

kvark · Apr 10, 2017

@icefoxen I haven't looked into rustfmt and there is no official policy. My rough understanding is that it's not ready for us yet, so no need to vigorously format anything :)

icefoxen · Apr 10, 2017

All right. I was more looking for ways to vigorously not reformat everything, since I usually code with rustfmt running automatically every time I save a file and it makes a terrible mess of projects that don't use it.

icefoxen · Apr 16, 2017

Okay, I think I've fixed all the formatting bugs and simplified where indicated. The only thing I haven't done is gotten rid of Cache::push() and altered the cache functions to return Option instead of maybe pushing the commands themselves. I can get rid of the former if anyone actually cares. I feel like the latter can work better because it gives the cache more control over what goes on, and makes the caller have to do less work. All the caller has to do is call the Cache's function to add a command.

I think that ideally every command should be added to the buffer via a method on Cache; if the cache can do nothing, it just pushes the command directly, but it would open the door to having more state be tracked by the Cache without having to change anything about how the caller operates.

kvark · Apr 17, 2017

@icefoxen thanks for updating it!!!

I feel like the latter can work better because it gives the cache more control over what goes on, and makes the caller have to do less work. All the caller has to do is call the Cache's function to add a command.
I think that ideally every command should be added to the buffer via a method on Cache; if the cache can do nothing, it just pushes the command directly, but it would open the door to having more state be tracked by the Cache without having to change anything about how the caller operates.

Don't worry about the caller. There is only one, so it doesn't make much sense to try to shove more logic into the Cache, making it sort of an opaque layer that fills up the command buffer (AFAIK, that's what your PR is doing). Thing is, CommandBuffer IS this layer already, so what Cache needs to be is just a structure holding some state, possibly with a few helpers if that make sense.

Does this sound reasonable?

icefoxen · Apr 18, 2017

Fair enough! It's hard sometimes, you look at stuff and say "hey, a layer of abstraction could go here!" and why not put one in?

This will probably take me a few days to get around to doing, but I'll refactor it so the functions just return an option.

icefoxen · Apr 19, 2017

Refactor complete; it IS nicer, thank you for the suggestion.

kvark · Apr 19, 2017

Thanks @icefoxen ! Looks great now! 👍
@homu r+

homu · Apr 19, 2017

📌 Commit 93febca has been approved by kvark

homu · Apr 19, 2017

⌛️ Testing commit 93febca with merge 90b563f...

homu · Apr 19, 2017

💔 Test failed - status

kvark · Apr 19, 2017

kvark approved these changes Apr 19, 2017

View changes

kvark · Apr 19, 2017

src/core/src/pso.rs

@@ -208,7 +208,7 @@ impl Descriptor {
 }

 /// A complete set of vertex buffers to be used for vertex import in PSO.
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone, Debug, PartialEq)]


are these core changes required for your PR?
We need to bump the version numbers and (if core is changed) change the dependency of core to the new version.

I'm afraid so because it needs to be able to compare the existing VertexBufferSet against the one it has cached. It can probably be worked around by just storing the contents of the VertexBufferSet, but that seems a little redundant.

Ok, I see. Please bump both version numbers then and make sure the new core is requested

kvark · Apr 19, 2017

src/backend/gl/src/command.rs

+    }
+
+    fn bind_program(&mut self, program: Program) -> Option<Command> {
+        if program == self.program {


nit: I think a simple if/else for the case of small branches should be preferred over return

kvark · Apr 21, 2017

Ok, we can bump the versions ourselves and do the refactor, if needed.

kvark · Apr 21, 2017

@icefoxen your code has sailed in https://crates.io/crates/gfx_device_gl/0.13.1

icefoxen referenced this pull request Apr 5, 2017
Closed
Attempts at making the GL backend cache more state #1220

Fix minor formatting and simplification issues as per PR

Loading status checks…

104869c

icefoxen referenced this pull request Apr 18, 2017
Closed
Drawing is *horribly* slow on Windows #31

Refactored command cache
It now only makes decisions and returns which commands to push, or None.

Loading status checks…

93febca

homu added a commit that referenced this pull request Apr 19, 2017

Auto merge of #1221 - icefoxen:gl-opt-take4, r=kvark
Adding more caching to GL backend. Like #1220 except based on the right starting point. Sorry.

Loading status checks…

90b563f

kvark merged commit 810eee4 into gfx-rs:v0.14 Apr 21, 2017
1 of 3 checks passed

1 of 3 checks passed

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

homu Test failed
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

kvark referenced this pull request Apr 21, 2017
Merged
Adding more caching to GL backend. (#1221) #1233

homu added a commit that referenced this pull request Apr 22, 2017

Auto merge of #1233 - kvark:gl-cache, r=kvark
Adding more caching to GL backend. (#1221) See #1221 from v0.14 branch

Loading status checks…

038cfa0

gfx-rs/gfx

Join GitHub today

Adding more caching to GL backend. #1221

Conversation

icefoxen commented Apr 5, 2017

icefoxen referenced this pull request Apr 5, 2017

kvark reviewed Apr 5, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

icefoxen commented Apr 10, 2017 • edited

This comment has been minimized.

kvark commented Apr 10, 2017

This comment has been minimized.

icefoxen commented Apr 10, 2017

This comment has been minimized.

icefoxen commented Apr 16, 2017 • edited

This comment has been minimized.

kvark commented Apr 17, 2017

This comment has been minimized.

icefoxen commented Apr 18, 2017

icefoxen referenced this pull request Apr 18, 2017

This comment has been minimized.

icefoxen commented Apr 19, 2017

This comment has been minimized.

kvark commented Apr 19, 2017

This comment has been minimized.

homu commented Apr 19, 2017

This comment has been minimized.

homu commented Apr 19, 2017

homu added a commit that referenced this pull request Apr 19, 2017

This comment has been minimized.

homu commented Apr 19, 2017

kvark approved these changes Apr 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark commented Apr 21, 2017

Hide details View details kvark merged commit 810eee4 into gfx-rs:v0.14 Apr 21, 2017 1 of 3 checks passed

1 of 3 checks passed

This comment has been minimized.

kvark commented Apr 21, 2017

kvark added a commit to kvark/gfx that referenced this pull request Apr 21, 2017

kvark referenced this pull request Apr 21, 2017

kvark added a commit to kvark/gfx that referenced this pull request Apr 22, 2017

homu added a commit that referenced this pull request Apr 22, 2017

kvark reviewed Apr 5, 2017

View changes

icefoxen commented Apr 10, 2017 •

edited

icefoxen commented Apr 16, 2017 •

edited

kvark approved these changes Apr 19, 2017

View changes

kvark merged commit `810eee4` into gfx-rs:v0.14 Apr 21, 2017
1 of 3 checks passed