Re: [Pixman] [PATCH v13 06/14] pixman-filter: reduce amount of malloc/free/memcpy to generate filter

[Bill Spitzak]

On Fri, Mar 4, 2016 at 2:52 PM, Søren Sandmann 
wrote:

>
>
> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> Rearranged so that the entire block of memory for the filter pair
>> is allocated first, and then filled in. Previous version allocated
>> and freed two temporary buffers for each filter and did an extra
>> memcpy.
>>
>> v8: small refactor to remove the filter_width function
>>
>> v10: Restored filter_width function but with arguments changed to
>>  match later patches
>>
>> v11: Removed unused arg and pointer from filter_width function
>>  Whitespace fixes.
>>
>> Signed-off-by: Bill Spitzak 
>> Reviewed-by: Oded Gabbay 
>>
>
> I'm not opposed to this patch if it is correct (and I'll believe the
> existing Reviewed-by for correctness), but I'm also don't think there is
> much point to this -- these malloc() and free() calls are unlikely to be
> performance bottlenecks.
>
> Acked-by: Soren Sandmann 
>

This is certainly correct, it would have been obvious in my tests if it did
not work.
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v13 06/14] pixman-filter: reduce amount of malloc/free/memcpy to generate filter

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> Rearranged so that the entire block of memory for the filter pair
> is allocated first, and then filled in. Previous version allocated
> and freed two temporary buffers for each filter and did an extra
> memcpy.
>
> v8: small refactor to remove the filter_width function
>
> v10: Restored filter_width function but with arguments changed to
>  match later patches
>
> v11: Removed unused arg and pointer from filter_width function
>  Whitespace fixes.
>
> Signed-off-by: Bill Spitzak 
> Reviewed-by: Oded Gabbay 
>

I'm not opposed to this patch if it is correct (and I'll believe the
existing Reviewed-by for correctness), but I'm also don't think there is
much point to this -- these malloc() and free() calls are unlikely to be
performance bottlenecks.

Acked-by: Soren Sandmann 


Søren
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH v13 06/14] pixman-filter: reduce amount of malloc/free/memcpy to generate filter

[spitzak]

From: Bill Spitzak 

Rearranged so that the entire block of memory for the filter pair
is allocated first, and then filled in. Previous version allocated
and freed two temporary buffers for each filter and did an extra
memcpy.

v8: small refactor to remove the filter_width function

v10: Restored filter_width function but with arguments changed to
 match later patches

v11: Removed unused arg and pointer from filter_width function
 Whitespace fixes.

Signed-off-by: Bill Spitzak 
Reviewed-by: Oded Gabbay 
---
 pixman/pixman-filter.c | 55 +-
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
index b70da1f..69ac3ab 100644
--- a/pixman/pixman-filter.c
+++ b/pixman/pixman-filter.c
@@ -217,23 +217,17 @@ integral (pixman_kernel_t reconstruct, double x1,
 }
 }
 
-static pixman_fixed_t *
-create_1d_filter (int *width,
+static void
+create_1d_filter (int  width,
  pixman_kernel_t  reconstruct,
  pixman_kernel_t  sample,
  double   size,
- int  n_phases)
+ int  n_phases,
+ pixman_fixed_t *p)
 {
-pixman_fixed_t *params, *p;
 double step;
 int i;
 
-*width = ceil (size * filters[sample].width + filters[reconstruct].width);
-
-p = params = malloc (*width * n_phases * sizeof (pixman_fixed_t));
-if (!params)
-return NULL;
-
 step = 1.0 / n_phases;
 
 for (i = 0; i < n_phases; ++i)
@@ -248,8 +242,8 @@ create_1d_filter (int *width,
 * and sample positions.
 */
 
-   x1 = ceil (frac - *width / 2.0 - 0.5);
-x2 = x1 + *width;
+   x1 = ceil (frac - width / 2.0 - 0.5);
+   x2 = x1 + width;
 
total = 0;
 for (x = x1; x < x2; ++x)
@@ -277,7 +271,7 @@ create_1d_filter (int *width,
 }
 
/* Normalize */
-   p -= *width;
+   p -= width;
 total = 1 / total;
 new_total = 0;
for (x = x1; x < x2; ++x)
@@ -289,10 +283,8 @@ create_1d_filter (int *width,
}
 
if (new_total != pixman_fixed_1)
-   *(p - *width / 2) += (pixman_fixed_1 - new_total);
+   *(p - width / 2) += (pixman_fixed_1 - new_total);
 }
-
-return params;
 }
 
 #ifdef PIXMAN_GNUPLOT
@@ -336,6 +328,12 @@ gnuplot_filter(int width, int samples, const 
pixman_fixed_t* p)
 }
 #endif
 
+static int
+filter_width (pixman_kernel_t reconstruct, pixman_kernel_t sample, double size)
+{
+return ceil (filters[reconstruct].width + size * filters[sample].width);
+}
+
 /* Create the parameter list for a SEPARABLE_CONVOLUTION filter
  * with the given kernels and size parameters
  */
@@ -352,42 +350,35 @@ pixman_filter_create_separable_convolution (int   
  *n_values,
 {
 double sx = fabs (pixman_fixed_to_double (size_x));
 double sy = fabs (pixman_fixed_to_double (size_y));
-pixman_fixed_t *horz = NULL, *vert = NULL, *params = NULL;
+pixman_fixed_t *params;
 int subsample_x, subsample_y;
 int width, height;
 
+width = filter_width (reconstruct_x, sample_x, sx);
 subsample_x = (1 << subsample_bits_x);
-subsample_y = (1 << subsample_bits_y);
 
-horz = create_1d_filter (, reconstruct_x, sample_x, sx, subsample_x);
-vert = create_1d_filter (, reconstruct_y, sample_y, sy, 
subsample_y);
+height = filter_width (reconstruct_y, sample_y, sy);
+subsample_y = (1 << subsample_bits_y);
 
-if (!horz || !vert)
-goto out;
-
 *n_values = 4 + width * subsample_x + height * subsample_y;
 
 params = malloc (*n_values * sizeof (pixman_fixed_t));
 if (!params)
-goto out;
+   return NULL;
 
 params[0] = pixman_int_to_fixed (width);
 params[1] = pixman_int_to_fixed (height);
 params[2] = pixman_int_to_fixed (subsample_bits_x);
 params[3] = pixman_int_to_fixed (subsample_bits_y);
 
-memcpy (params + 4, horz,
-   width * subsample_x * sizeof (pixman_fixed_t));
-memcpy (params + 4 + width * subsample_x, vert,
-   height * subsample_y * sizeof (pixman_fixed_t));
+create_1d_filter (width, reconstruct_x, sample_x, sx, subsample_x,
+ params + 4);
+create_1d_filter (height, reconstruct_y, sample_y, sy, subsample_y,
+ params + 4 + width * subsample_x);
 
 #ifdef PIXMAN_GNUPLOT
 gnuplot_filter(width, subsample_x, params+4);
 #endif
 
-out:
-free (horz);
-free (vert);
-
 return params;
 }
-- 
1.9.1

___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman