On 17 May 2012 11:21, Ian Romanick <i...@freedesktop.org
<mailto:i...@freedesktop.org>> wrote:
On 05/14/2012 06:24 PM, Paul Berry wrote:
On 14 May 2012 16:50, Ian Romanick <i...@freedesktop.org
<mailto:i...@freedesktop.org>
<mailto:i...@freedesktop.org <mailto:i...@freedesktop.org>>> wrote:
On 05/14/2012 02:13 PM, Paul Berry wrote:
Just to update the list: I had an in-person meeting with
Eric,
Ian, and
Ken today to discuss the design of the patch series and
decide
what to
do about it. We agreed to go ahead and push the patches to
master as
is, and to make further improvements as time goes on.
Specific improvements that were discussed:
- I seem to be the only person who thinks it makes sense
for the
brw_blorp_params class to contain an exec() function.
In the
interest
of avoiding a rebasing nightmare, I will go ahead with the
patches as
they are, and submitt a follow up patch which replaces
brw_blorp_params::exec() with a global function
brw_blorp_exec(const
brw_blorp_params *).
Part of the issue was that "exec" isn't a terrific name, and
exec'ing a set of parameters doesn't have implicit meaning. My
preference was for a function, similar to what you mention,
called
brw_blorp_do_blit or similar.
I'm not going to try to change your mind, but in the spirit of
clarifying the method that was behind my madness:
I did not have the clarity of mind during our discussion today
to point
out that in creating the brw_blorp_params class, I was in fact
trying to
follow a standard design pattern, the Gang of Four "command" pattern
(http://en.wikipedia.org/wiki/__Command_pattern
<http://en.wikipedia.org/wiki/Command_pattern>). The idea of that
There are some other places, mostly in the compiler, where various
patterns are employed. In those cases, I've explicitly put in a
comment like, "This is pattern XYZ. Refer to http://foo/ for the
back story." The comment block in
src/glsl/ir_hierarchical___visitor.h is an example.
pattern is to encapsulate all of the information necessary to
perform an
action into a class; this allows the action to be treated as a first
class object. The usual reasons why it might be useful for the
action
to be a first class object are so that it can be stored in an undo
history, so that its execution can be deferred, restarted, or
repeated,
or so that some other part of the program can take the
opportunity to
tweak the parameters before the action is performed. In this
particular
case the benefit is that the various phases of the action can be
performed by different functions without an explosion of parameter
passing, and so that actions that share some, but not all, of their
implementation (HiZ ops and blits, in this case) can be
implemented by
using a base class with virtual functions. It's also possible
that in
the future, the blorp engine might be able to record the last
command it
executed, so that it can compare that to the command it's being
asked to
execute and avoid some redundant pipeline setup. In the command
pattern, it is conventional to make the function that performs the
action a member of the command class and to call it "exec" or
"execute". So from my point of view, this function name
actually *does*
have an implicit meaning. I'm aware, of course, that most of my
fellow
Mesa developers don't share my respect for Gang of Four-style
design :)
(Note that in a strict implementation of the "command" pattern
there are
other auxiliary classes involved, but I thought that would be
way too
heavyweight for this purpose).
I believe that it is also typical in the command pattern for the
classes to have "command" in the name instead of "parameters." At
least the examples in http://en.wikipedia.org/wiki/__Command_pattern
<http://en.wikipedia.org/wiki/Command_pattern> and my "Patterns in
Java" book suggest this.
For me, the advantage of patterns is rarely technical. For me, the
advantage of patterns is in communication with other humans. If a
piece of code says, "I'm an implementation of the factory pattern"
people will have a set of expectations about what's coming. In some
sense they'll be familiar with the code before they even see it.
There was, by necessity, a lot of moving parts in MSAA patch series.
That combined with a slight deviation from the pattern (the
previously mentioned parameters vs. command naming) caused me to not
see the pattern. For me, that defeated the improved communication
of pattern use.
Communication isn't just with other humans. Perhaps if the class
had been called brw_blorp_blit_command you would have remembered
that you were using the command pattern while we were having the
review meeting. :)
Yes, you're right. That would have helped.
If I see foo->exec() or exec(foo) in a pipe of code, I have
to go
look at the declaration of foo to know what's going on. If
I see
do_some_action(foo) it's a bit more obvious.
In this particular case (and in most uses of the command
pattern, IMHO),
the declaration is right next to the exec() call, so this isn't a
problem. E.g.:
/* Do the blit */
brw_blorp_blit_params params(brw_context(ctx), src_mt, dst_mt,
srcX0, srcY0, dstX0, dstY0,
dstX1, dstY1,
mirror_x, mirror_y);
params.exec(intel);
I agree that it looks kind of hokey, though. Other alternatives I
considered included:
- Make intel_context a class, and make executing a blorp action
a method
on that class, so that last line would have been something like
"intel->do_blorp_op(params);". Incidentally, I think we would
get a lot
of benefits out of making brw_context, intel_context, and
gl_context a
class hierarchy; for example, macros like OUT_BATCH() would become
ordinary member functions, and we wouldn't have to do anywhere
near as
many explicit conversions between the three context types.
Also, the
virtual function tables that we currently code up explicitly
would be
automatically created by the compiler, so we would have much
less risk
of problems like forgetting to dispatch through the virtual function
table, forgetting to initialize a function pointer properly, or
initializing a function pointer at the wrong time. Of course, this
would have been *way* too big a change to try to slip into this
patch
series. By orders of magnitude.
- Make a base class to represent the Blorp engine, with a
derived class
for Gen6 and a derived class for Gen7. The appropriate object
would be
created at context creation time. Then, instead of calling
"params.exec(intel);", you would do "brw->blorp->exec(params);". I
largely dismissed this option because I had already done so much
refactoring of the HiZ code that it seemed like it was time to
settle
down and actually implement some new functionality. But I would
still
be open to it if people like the idea.
Wouldn't a more usual way to handle this kind of partitioning be
with an abstract factory?
Can you elaborate? IIRC, an abstract factory is for when component A
needs to do object creation, but component B knows how to do object
creation, so you create an abstract factory class with a pure virtual
"create" method. Component B instantiates a concrete factory and hands
it off to component A, and then component A can use the factory to
create the objects without needing to know details about how they are
created. It's not obvious to me how to map this pattern onto the
situation we have with brw_blorp_params.
