Review: compaction as we build closures

In practice with our production shaders, were were often getting Ci's with many components that were duplicates.  We figured this wasn't great for performance and also wouldn't be the best input for our integrator.  So I've modified the "closure add" to check if components are duplicates, and if so merge them by adding the weights rather than appending.  Also to not bother merging if the weights are 0.

Results:

Baseline: 

	4:45  1427 MB
	Largest Ci closure constructed: 12 components
	Tiny weight components: 7037131 / 128325701 (5.4838% of components, present in 3.48453% of Ci's)
	Zero weight components: 6719907 / 128325701 (5.2366%) 


After my changes:

	4:32  1403 MB
	Largest Ci closure constructed: 4 components
	Tiny weight components: 42956 / 113090187 (0.0379838% of components, present in 0.0426577% of Ci's)
	Zero weight components: 0 / 113090187 (0%)


So it's a tad faster, an eensy bit less memory, and the images look just a bit less noisy now.

[aconty, 2010-03-25 23:38:40]

http://codereview.appspot.com/766041/diff/1/7
File src/liboslexec/bsdf_diffuse.cpp (right):

http://codereview.appspot.com/766041/diff/1/7#newcode51
src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
base_mergeable(other);
Wouldn't it be better to just call the parent's method rather than defining
base_mergeable?

[lg_imageworks.com, 2010-03-26 03:08:58]

On Mar 25, 2010, at 4:38 PM, <aconty@gmail.com> <aconty@gmail.com> wrote:
> 
> http://codereview.appspot.com/766041/diff/1/7#newcode51
> src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
> base_mergeable(other);
> Wouldn't it be better to just call the parent's method rather than
> defining base_mergeable?

But then if somebody forgets to provide a closure subclass with a mergeable
method, it will revert to the parent class, which will be wrong, wrong, wrong,
it will say it's mergeable without being aware of any subclass-specific
parameters.  I wanted the base class's method to either be "return false" (never
merge, always correct even if the subclass fails to provide the method) or to be
pure virtual (force the subclasses to provide the method).

And yet, I still needed a way for the subclasses to say "and also check the
stuff from the parent class".

So that's how I ended up with a pure virtual mergeable() in the base class, and
a separate method called base_mergeable().

If you have a more elegant idea, I'm all ears.

--
Larry Gritz
lg@imageworks.com

[Chris Foster, 2010-03-26 03:38:29]

On Fri, Mar 26, 2010 at 1:08 PM, Larry Gritz <lg@imageworks.com> wrote:
> On Mar 25, 2010, at 4:38 PM, <aconty@gmail.com> <aconty@gmail.com> wrote:
>>
>> http://codereview.appspot.com/766041/diff/1/7#newcode51
>> src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
>> base_mergeable(other);
>> Wouldn't it be better to just call the parent's method rather than
>> defining base_mergeable?
>
> But then if somebody forgets to provide a closure subclass with a mergeable
> method, it will revert to the parent class, which will be wrong, wrong,
> wrong, it will say it's mergeable without being aware of any
> subclass-specific parameters.  I wanted the base class's method to either
> be "return false" (never merge, always correct even if the subclass fails to
> provide the method) or to be pure virtual (force the subclasses to provide
> the method).
>
> And yet, I still needed a way for the subclasses to say "and also check the
> stuff from the parent class".
>
> So that's how I ended up with a pure virtual mergeable() in the base class,
> and a separate method called base_mergeable().

Actually, it's possible to provide a base class implementation of a pure
virtual function for exactly the purpose of forcing an override while
providing some default behaviour (how obtuse!).  The following compiles:

struct A
{
    virtual int foo() = 0;
};

int A::foo()
{
    return 42;
}

struct B : public A
{
    virtual int foo()
    {
        return A::foo();
    }
};

I'm not sure I'd claim this is any more elegant ;-)


One alternative which might be nicer (but OTOH not fully overridable)
is to keep the two-method design but invert it - have the base closure
mergeable() call the child class version; something like

class ClosurePrimitive {
public:
    bool mergeable (const ClosurePrimitive *other) const;
    {
        bool base_mergeable = /* stuff from current base_mergeable () */;
        return base_mergeable && derived_mergeable(other);
    };

    //...

protected:
    virtual bool derived_mergeable (const ClosurePrimitive *other) const = 0;
};


that way the child classes can't forget to call the base_mergeable() logic.

~Chris

[ckulla, 2010-03-26 15:56:11]

I second Chris's suggestion of the "inverted" two method setup.

[ckulla, 2010-03-26 16:05:18]

http://codereview.appspot.com/766041/diff/1/7
File src/liboslexec/bsdf_diffuse.cpp (right):

http://codereview.appspot.com/766041/diff/1/7#newcode51
src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
base_mergeable(other);
What about the sidedness parameter in the BSDFClosure class? It doesn't seem to
be checked.

http://codereview.appspot.com/766041/diff/1/3
File src/liboslexec/closure.cpp (right):

http://codereview.appspot.com/766041/diff/1/3#newcode154
src/liboslexec/closure.cpp:154: c.weight *= w;
It is still possible to end up with 0 weights here:

Ci = diffuse(N);
Ci *= color(1, 0, 0);
Ci *= color(0, 1, 0);

(not sure if this is a problem, but we should remember the code doesn't
guarantee the weights to be non-zero)

[aconty, 2010-03-26 17:34:00]

Just to give another alternative, leave it as it is but make base_mergeable
virtual so we allow for chained method call on the one too. So ClosurePrimitive
would have one and BSDFClosure another (doing the sidedness check) that would in
addition call the parent.

[larrygritz, 2010-03-26 17:44:16]

Use mergeable/derived_mergeable as suggested.

[lg_larrygritz.com, 2010-03-26 17:47:48]

On Mar 26, 2010, at 9:05 AM, ckulla@gmail.com wrote:
> 
> http://codereview.appspot.com/766041/diff/1/7#newcode51
> src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
> base_mergeable(other);
> What about the sidedness parameter in the BSDFClosure class? It doesn't
> seem to be checked.

Right.  Hang on, I'll fix that.


> http://codereview.appspot.com/766041/diff/1/3#newcode154
> src/liboslexec/closure.cpp:154: c.weight *= w;
> It is still possible to end up with 0 weights here:
> 
> Ci = diffuse(N);
> Ci *= color(1, 0, 0);
> Ci *= color(0, 1, 0);
> 
> (not sure if this is a problem, but we should remember the code doesn't
> guarantee the weights to be non-zero)

I was not intending to get rid of all possible zero-weighted components. 
Integrators should be robust to that.  But I did put in a quick check to catch
the common cases so we don't usually let the closures grow just to add zero
components.

--
Larry Gritz
lg@larrygritz.com

[larrygritz, 2010-03-26 18:28:20]

Third time's the charm?

[aconty, 2010-03-26 21:42:39]

LGTM, just one comment in case you want to consider it for this commit

http://codereview.appspot.com/766041/diff/28001/29001
File src/include/oslclosure.h (right):

http://codereview.appspot.com/766041/diff/28001/29001#newcode118
src/include/oslclosure.h:118: virtual const char * name () const = 0;
Why didn't you make this a ustring? I'm a ustring fan

[ckulla, 2010-03-26 21:48:37]

LGTM2

A few small comments:

http://codereview.appspot.com/766041/diff/28001/29001
File src/include/oslclosure.h (right):

http://codereview.appspot.com/766041/diff/28001/29001#newcode181
src/include/oslclosure.h:181: for (int i = 0;  i <= MAXCUSTOM;  ++i) {
The last one is guaranteed to be Labels::NONE, so no need to check it.

http://codereview.appspot.com/766041/diff/28001/29001#newcode212
src/include/oslclosure.h:212: return m_sidedness == comp->m_sidedness &&
In theory only m_sidedness is a user settable parameter, the other ones are
fixed by the type of closure.