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.

Sign up for GitHub

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

Don't merge layers that produce renderer outputs. #1296

Merged
merged 1 commit into from
Nov 20, 2020
Merged

Don't merge layers that produce renderer outputs. #1296

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/liboslexec/instance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,11 @@ ShaderInstance::mergeable (const ShaderInstance &b, const ShaderGroup& /*g*/) co
if (master() != b.master())
return false;

// If one or both instances are directly hooked up to renderer outputs,
// don't merge them.
if (renderer_outputs() || b.renderer_outputs())
Copy link
Contributor

Choose a reason for hiding this comment

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

I still can't quite decide if this is in any way responsible for the current production issues I'm having, and this code looks very different now to the OSL version we are using, but merging the renderer outputs in the version I am using causes an assertion. That doesn't seem to be the case in this code.

I don't really know enough about the internals to understand the consequences of this, but happy to thumbs up this if you are sure this is necessary @lgritz thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure it's necessary, but I strongly suspect it is. Here's my thinking.

Let's suppose that we have two nodes A and B in the shader group, which are identical except for whether they are directly connected to any shader outputs.

Case 1: which outputs they are connected to are different. Then it seems to me that merging is a bad idea, because only one of them can win, somebody's outputs will never get set. Suppose the app has asked for A.x to go to one output and B.x to go to a different output. The post-merged shader can't be both A and B. It has to be one, with the assumption that the other is identical. So one output will be lost. That has to be wrong, so we shouldn't merge.

Case 2: the outputs are the same. MAYBE it's safe to merge, if it's a pure output and both shader are in all other ways identical so they will get the same value. Then it doesn't matter if they are collapsed, the same thing happens if A runs, then B, or if B runs, then A, or if only one of them runs. But what if the shader is one of these "pass-through with modification" (myoutput *= 2), then merging them and running it once is clearly a different result than running it twice. So again, we should not merge.

Now, there is the small danger that in some cases it was perfectly safe to merge (the safe pure output sub-case of case 2), and now we'll end up running the node twice because we are no longer merging a case that we used to merge. I'm going to argue that that this doesn't happen often enough to worry about. Most shader nodes are not hooked up to renderer outputs at all. Usually the shader graph computes all sorts of things and feeds into just one node at the "root" (or back end) that directly hooks up to the outputs, so there will tend not be two of them. If there are multiple nodes connected to renderer outputs, they will tend NOT to be exactly duplicates. And if they are, it's so ill-advised and maybe ill-defined (for the reasons I outlined above), I'm not going to concern myself with any slight loss of performance by no longer merging them.

If you think this is sound reasoning, feel free to go ahead and approve the PR. It may not be responsible for your current issue, but I do think we were right to question the validity of merging output-generating nodes.

return false;

// If the shaders haven't been optimized yet, they don't yet have
// their own symbol tables and instructions (they just refer to
// their unoptimized master), but they may have an "instance
Expand Down