On Wednesday 12 January 2011 12:46:39 Soeren Sandmann wrote: > Siarhei Siamashka <siarhei.siamas...@gmail.com> writes: > > > 15 files changed, 743 insertions(+), 470 deletions(-) > > > > On the other hand, I'm not so happy about the implementation. The number > > of lines of code as well as the complexity has increased, and the > > performance seems to have dropped a little bit in some of the cases. > > The performance numbers you posted seem very close to noise to me; in > some cases the new code is faster. Considering that this was measured > with the CPU specific fast paths turned off, most users won't actually > see even this small slowdown.
Performance numbers for some of the tests look like they are more or less statistically relevant. It's not a big slowdown, but if such changes keep accumulating, then we got a problem. > > I really want to see some practical benefits which depend on these > > added iterators in the near future. > > It isn't clear to me what specifically you are suggesting. More like I'm trying to figure out the purpose of all these changes and their place in the long term plans. If this code is only related to general implementation which just needs to be simple, clean and maintainable, then it is perfectly fine. If the iterators are also going to be used in the fast path functions eventually, then they are better to be also really fast. > One practical benefit of the patch set is that it fixes the bug where > destination properties aren't ignored as they should be. This bug > could probably be fixed some other way, but it doesn't seem to be > possible with separating source- and destination accessors in some > way. This separation is where most of the additional new lines are > coming from. > > In fact, as far as I can tell, the iterators make the code _less_ > complex. Compare the new loop in pixman-general.c: > > for (i = 0; i < height; ++i) > { > uint32_t *s, *m, *d; > > m = mask_iter.get_scanline (&mask_iter, NULL); > s = src_iter.get_scanline (&src_iter, m); > d = dest_iter.get_scanline (&dest_iter, NULL); > > compose (imp->toplevel, op, d, s, m, width); > > dest_iter.write_back (&dest_iter); > } > > with the old one: > > for (i = 0; i < height; ++i) > { > /* fill first half of scanline with source */ > if (fetch_src) > { > if (fetch_mask) > { > /* fetch mask before source so that fetching of > source can be optimized */ > fetch_mask (mask, mask_x, mask_y + i, > width, (void *)mask_buffer, 0); > > if (mask_class == SOURCE_IMAGE_CLASS_HORIZONTAL) > fetch_mask = NULL; > } > > if (src_class == SOURCE_IMAGE_CLASS_HORIZONTAL) > { > fetch_src (src, src_x, src_y + i, > width, (void *)src_buffer, 0); > fetch_src = NULL; > } > else > { > fetch_src (src, src_x, src_y + i, > width, (void *)src_buffer, (void > *)mask_buffer); > } > } > else if (fetch_mask) > { > fetch_mask (mask, mask_x, mask_y + i, > width, (void *)mask_buffer, 0); > } > > if (store) > { > /* fill dest into second half of scanline */ > if (fetch_dest) > { > fetch_dest (dest, dest_x, dest_y + i, > width, (void *)dest_buffer, 0); > } > > /* blend */ > compose (imp->toplevel, op, > (void *)dest_buffer, > (void *)src_buffer, > (void *)mask_buffer, > width); > > /* write back */ > store (&(dest->bits), dest_x, dest_y + i, width, > (void *)dest_buffer); > } > else > { > /* blend */ > compose (imp->toplevel, op, > bits + (dest_y + i) * stride + dest_x, > (void *)src_buffer, (void *)mask_buffer, width); > } > } The old code had more lines of code for this part, but on the other hand all this code was in a single place as opposed to being spread around. In some ways, the new code is a bit more limiting. For example, fetching directly to destination without intermediate buffers for SRC operator might be more tricky to implement. Overall, the patch set looks good. It's just not everything seems to be rosy to me, I can see (and accept) some of the compromises it introduces. -- Best regards, Siarhei Siamashka _______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman