[Pixman] [PATCH 2/2] Delete simple repeat code

[Siarhei Siamashka]

On Thursday 16 December 2010 15:58:22 Soeren Sandmann wrote:
> Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:
> > > As it turns out, non-scaled images having NORMAL repeat (tiled
> > > pictures) are actually used occasionally (typically with SRC
> > > operator). And removing simple repeat code caused ~4x performance
> > > regression in these cases. Looks like a new fast path is needed to
> > > solve the problem.
> > 
> > Just an update to this. I tried to check whether this is easily solvable,
> > but looks like there are a handful of missing fast paths for NORMAL
> > repeat (they primarily show up when browsing various pages). Just doing
> > it in a simple and straightforward way by adding all of them as new fast
> > path functions may cause more redundancy in the pixman source code.
> > Modifying the existing scaled fast paths to accept non-scaled cases also
> > works, but it does not provide the best performance.
> 
> An idea might be to move the lookup_fast_path() into pixman-utils.c,
> and then write a new fast path that would call this function to look
> up another fast path and call it through the simple repeat code.
> 
> This would require the full set of flags to be passed down to the fast
> paths, but we need that for the performance reporting anyway.
> 
> Just a suggestion, I don't know whether it would work in practice.

Sounds reasonable, and I think it should work fine, just because it is not
much different from the old simple repeat code. But surely it's going to be  
cleaner this way.

This new fast path can start with basically pulling in the old simple repeat
code, but evolve into something more cache friendly later, still using the
existing optimizations for rectangles processing as optimization primitives.

> (An interesting possibility from this would be to use the regular
> blitters as the "combiners" in the general code. That way, if you
> have, say, a
> 
>         gradient IN a8 OVER 565
> 
> operation, then instead of converting both a8 and 565 to 8888, an
> existing over_8888_8_565 function could just be called with pointers
> directly into the mask and destination. Some changes to the general
> code would obviously be required though).

That's a good idea, but call overhead of the existing fast paths may be too
high. Even right now, just adding a new (mostly empty) implementation shows a 
small, but measurable performance regression which I observed when trying
to benchmark MIPS optimizations. This happens because we are walking through
combiners delegate chain for each scanline. I personally think that
'delegate_combine_32' is evil and should be eventually removed. And increasing  
call overhead in any other ways is better to be avoided too.

More lightweight fast paths with smaller call overhead are probably nothing
else than the existing combiners. Surely the way how they are looked up can be 
changed to be the same as fast paths. But from the practical point of view, the 
thing that is really worth optimizing here is a8 mask, because it is important 
for all platforms (565 is only relevant for the embedded platforms, the other 
formats are probably not relevant at all). Could the standard combiners be 
changed to work with a8 mask, instead of a8x8x8x8 as it is now?

> > For now I just reverted this patch in my pixman tree to workaround
> > the problem and buy some more time to think about how it can be
> > solved.
> 
> Does reverting the patch actually work? The standard flags currently
> include SAMPLES_COVER_CLIP, and for a typical repeating blit, that
> would not be set.

Yes, setting FAST_PATH_SAMPLES_COVER_CLIP together with FAST_PATH_SIMPLE_REPEAT
is a part of the simple repeat removal reversal. This brings back the 
consistency issues regarding flags interpretation, but as long as this flags 
pair is only set for non-transformed images with NORMAL repeat, it never hits a 
wrong fast path (yet). 

-- 
Best regards,
Siarhei Siamashka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20101217/d992c9df/attachment.pgp>