Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Oct 7, 2023

Hacky mutable stuff, but avoids recomputing string keys and hashing them multiple times per draw call. See also

While we cache the uniform location, looking up this value requires computing a string key for every single uniform value on every single command. We can speed this up by stashing the uniform location in the shader metadata, which shouldn't change for the app lifetime. This avoids both the string computation and the hashmap usage.

@jonahwilliams jonahwilliams marked this pull request as ready for review October 10, 2023 03:08
/// the OpenGLES backend.
///
/// See buffer_binding_gles.cc.
mutable std::optional<int> location = std::nullopt;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me. But when this was written, the metadata was meant to be static const. So I am not sure about the lifecycle of this. I am also worried (perhaps incorrectly) about interactive with multiple OpenGL drivers at some point in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Triage: lets just store it in the "PSO" instead.

@jonahwilliams
Copy link
Contributor Author

I've updated this to cache the binding data in in the buffer_binding, I couldn't put them in the pipeline itself as that doesn't actually have all of the information needed. To support this I made the buffer_binding pointer non-const.

So we still use one hashmap lookup per shader, but the key we use is already generated on the shader metadata so its less work.

Copy link
Contributor

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't tell what information was missing to put this in the pipeline state object. But this is an improvement for sure.

for (auto i = 0u; i < metadata->members.size(); i++) {
const auto& member = metadata->members[i];
auto location = locations[i];
if (location == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserve the comment about inactive uniformed being optimized out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see it up top. Maybe just "void type or inactive".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jonahwilliams
Copy link
Contributor Author

The BufferBinding is essentially part of the PSO, so its all approximately the same, right 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants