[cairo] Returning cached erroneous patterns
Chris Wilson
chris at chris-wilson.co.uk
Mon Mar 26 02:43:29 PDT 2007
Behdad Esfahbod (behdad at behdad.org) said:
> Ok, thanks for raising the issue. The problem is that solid patterns
> are not immutable as we expected. Giving away the same mutable object
> multiple times is definitely wrong.
>
> I wanted to say we should back out the cache, but then I found that the
> fix is really easy: instead of reusing patterns from the cache, keep a
> pool of freed solid patterns. That is: when using a pattern from the
> cache, remove it from the cache, and when freeing a solid pattern of
> refcount 1, set its error status to SUCCESS and add it to the cache.
So we back out the solid-pattern-cache and replace it with a recently
freed cache. We then add one further refinement Behdad suggested which
is to destroy the current pattern before creating a new solid-pattern in
the likelihood that we will immediately reuse the freed pattern.
--
Chris Wilson
-------------- next part --------------
>From d2c91b1a7289778cea9531cb9ad6ace38961a400 Mon Sep 17 00:00:00 2001
From: Chris Wilson <chris at chris-wilson.co.uk>
Date: Mon, 26 Mar 2007 10:12:46 +0100
Subject: [PATCH] Back out the solid-pattern-cache.
Unfortunately one cannot cache live patterns and return a fresh reference
instead of creating new ones as patterns can be modified by the user and
so cannot be transparently shared between different users. However,
solid colour allocation is still a frequent operation, so we maintain a
small cache of recently freed patterns to reduce the malloc pressure.
---
src/cairo-pattern.c | 100 ++++++++++++++++++++++++--------------------------
1 files changed, 48 insertions(+), 52 deletions(-)
diff --git a/src/cairo-pattern.c b/src/cairo-pattern.c
index 920542c..ed535e1 100644
--- a/src/cairo-pattern.c
+++ b/src/cairo-pattern.c
@@ -259,73 +259,49 @@ _cairo_pattern_init_radial (cairo_radial_pattern_t *pattern,
pattern->gradient.c2.radius = _cairo_fixed_from_double (fabs (radius1));
}
-/* We use a small cache here, because we don't want to constantly
- * reallocate simple colors. */
-#define MAX_PATTERN_CACHE_SIZE 16
+/* We use a small freed pattern cache here, because we don't want to
+ * constantly reallocate simple colors. */
+#define MAX_PATTERN_CACHE_SIZE 4
static struct {
- struct {
- cairo_color_t color;
- cairo_solid_pattern_t *pattern;
- } cache[MAX_PATTERN_CACHE_SIZE];
+ cairo_solid_pattern_t *patterns[MAX_PATTERN_CACHE_SIZE];
int size;
} solid_pattern_cache;
static cairo_pattern_t *
-_cairo_pattern_create_solid_not_cached (const cairo_color_t *color)
+_cairo_pattern_create_solid_from_cache (const cairo_color_t *color)
{
- void *pattern;
+ cairo_solid_pattern_t *pattern = NULL;
- /* Not cached, need to create a new pattern. */
- pattern = malloc (sizeof (cairo_solid_pattern_t));
+ CAIRO_MUTEX_LOCK (_cairo_pattern_solid_cache_lock);
+
+ if (solid_pattern_cache.size) {
+ int i = --solid_pattern_cache.size %
+ ARRAY_LEN (solid_pattern_cache.patterns);
+ pattern = solid_pattern_cache.patterns[i];
+ solid_pattern_cache.patterns[i] = NULL;
+ }
+
+ CAIRO_MUTEX_UNLOCK (_cairo_pattern_solid_cache_lock);
+
+ if (pattern == NULL) {
+ /* None cached, need to create a new pattern. */
+ pattern = malloc (sizeof (cairo_solid_pattern_t));
+ }
if (pattern != NULL)
_cairo_pattern_init_solid (pattern, color);
- return pattern;
+ return &pattern->base;
}
cairo_pattern_t *
_cairo_pattern_create_solid (const cairo_color_t *color)
{
- static int cache_index;
- void *pattern;
-
- CAIRO_MUTEX_LOCK (_cairo_pattern_solid_cache_lock);
-
- /* Check cache first. */
- if (cache_index < solid_pattern_cache.size &&
- _cairo_color_equal (
- &solid_pattern_cache.cache[cache_index].color, color))
- goto DONE;
-
- for (cache_index = 0; cache_index < solid_pattern_cache.size; cache_index++)
- if (_cairo_color_equal (
- &solid_pattern_cache.cache[cache_index].color, color))
- goto DONE;
+ cairo_pattern_t *pattern;
- /* Not cached, need to create a new pattern. */
- pattern = _cairo_pattern_create_solid_not_cached (color);
+ pattern = _cairo_pattern_create_solid_from_cache (color);
if (pattern == NULL)
return (cairo_pattern_t *) &cairo_pattern_nil.base;
- /* And insert it into the cache. */
- if (solid_pattern_cache.size < MAX_PATTERN_CACHE_SIZE) {
- solid_pattern_cache.size ++;
- /* cache_index == solid_pattern_cache.size */
- } else {
- /* Evict an old pattern. */
- cache_index = rand () % MAX_PATTERN_CACHE_SIZE;
- cairo_pattern_destroy (
- &solid_pattern_cache.cache[cache_index].pattern->base);
- }
-
- solid_pattern_cache.cache[cache_index].color = *color;
- solid_pattern_cache.cache[cache_index].pattern = pattern;
-
-DONE:
- pattern = cairo_pattern_reference (
- &solid_pattern_cache.cache[cache_index].pattern->base);
- CAIRO_MUTEX_UNLOCK (_cairo_pattern_solid_cache_lock);
-
return pattern;
}
@@ -336,8 +312,10 @@ _cairo_pattern_reset_static_data (void)
CAIRO_MUTEX_LOCK (_cairo_pattern_solid_cache_lock);
- for (i = 0; i < solid_pattern_cache.size; i++)
- cairo_pattern_destroy (&solid_pattern_cache.cache[i].pattern->base);
+ for (i = 0; i < MIN (ARRAY_LEN (solid_pattern_cache.patterns), solid_pattern_cache.size); i++) {
+ free (solid_pattern_cache.patterns[i]);
+ solid_pattern_cache.patterns[i] = NULL;
+ }
solid_pattern_cache.size = 0;
CAIRO_MUTEX_UNLOCK (_cairo_pattern_solid_cache_lock);
@@ -348,7 +326,7 @@ _cairo_pattern_create_in_error (cairo_status_t status)
{
cairo_pattern_t *pattern;
- pattern = _cairo_pattern_create_solid_not_cached (_cairo_stock_color (CAIRO_STOCK_BLACK));
+ pattern = _cairo_pattern_create_solid_from_cache (_cairo_stock_color (CAIRO_STOCK_BLACK));
if (cairo_pattern_status (pattern))
return pattern;
@@ -648,7 +626,25 @@ cairo_pattern_destroy (cairo_pattern_t *pattern)
return;
_cairo_pattern_fini (pattern);
- free (pattern);
+
+ /* maintain a small cache of freed patterns */
+ if (pattern->type == CAIRO_PATTERN_TYPE_SOLID) {
+ int i;
+
+ CAIRO_MUTEX_LOCK (_cairo_pattern_solid_cache_lock);
+
+ i = solid_pattern_cache.size++ %
+ ARRAY_LEN (solid_pattern_cache.patterns);
+ /* swap an old pattern for this 'cache-hot' pattern */
+ if (solid_pattern_cache.patterns[i])
+ free (solid_pattern_cache.patterns[i]);
+
+ solid_pattern_cache.patterns[i] = (cairo_solid_pattern_t *) pattern;
+
+ CAIRO_MUTEX_UNLOCK (_cairo_pattern_solid_cache_lock);
+ } else {
+ free (pattern);
+ }
}
slim_hidden_def (cairo_pattern_destroy);
--
1.4.4.2
-------------- next part --------------
>From 8b884ec31a92d912871eebe4a6563174a5d68c7c Mon Sep 17 00:00:00 2001
From: Chris Wilson <chris at chris-wilson.co.uk>
Date: Mon, 26 Mar 2007 10:33:32 +0100
Subject: [PATCH] Destroy the current pattern before replacing with cairo_set_source().
Frequently cairo_set_source_rgb[a]() is used to replace the current
solid-pattern source with a new one of a different colour. The current
pattern is very likely to be unshared and unmodified and so it is likely
just to be immediately freed [or rather simply moved to recently freed
cache]. However as the last active pattern it is likely to cache-warm and
suitable to satisfy the forthcoming allocation. So by setting the current
pattern to 'none' we can move the pattern to the freed list before we
create the new pattern and hopefully immediately reuse it.
---
src/cairo-pattern.c | 10 ++++++++++
src/cairo.c | 9 +++++++++
src/cairoint.h | 1 +
3 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/src/cairo-pattern.c b/src/cairo-pattern.c
index ed535e1..2bc5580 100644
--- a/src/cairo-pattern.c
+++ b/src/cairo-pattern.c
@@ -50,6 +50,16 @@ static const cairo_solid_pattern_t cairo_pattern_nil_null_pointer = {
CAIRO_EXTEND_GRADIENT_DEFAULT }, /* extend */
};
+const cairo_solid_pattern_t cairo_pattern_none = {
+ { CAIRO_PATTERN_TYPE_SOLID, /* type */
+ CAIRO_REF_COUNT_INVALID, /* ref_count */
+ CAIRO_STATUS_SUCCESS, /* status */
+ { 0, 0, 0, NULL }, /* user_data */
+ { 1., 0., 0., 1., 0., 0., }, /* matrix */
+ CAIRO_FILTER_DEFAULT, /* filter */
+ CAIRO_EXTEND_GRADIENT_DEFAULT }, /* extend */
+};
+
/**
* _cairo_pattern_set_error:
* @pattern: a pattern
diff --git a/src/cairo.c b/src/cairo.c
index 22e15fd..9a96dda 100644
--- a/src/cairo.c
+++ b/src/cairo.c
@@ -671,6 +671,9 @@ cairo_set_source_rgb (cairo_t *cr, double red, double green, double blue)
if (cr->status)
return;
+ /* push the current pattern to the freed lists */
+ cairo_set_source (cr, (cairo_pattern_t *) &cairo_pattern_none);
+
pattern = cairo_pattern_create_rgb (red, green, blue);
cairo_set_source (cr, pattern);
cairo_pattern_destroy (pattern);
@@ -702,6 +705,9 @@ cairo_set_source_rgba (cairo_t *cr,
if (cr->status)
return;
+ /* push the current pattern to the freed lists */
+ cairo_set_source (cr, (cairo_pattern_t *) &cairo_pattern_none);
+
pattern = cairo_pattern_create_rgba (red, green, blue, alpha);
cairo_set_source (cr, pattern);
cairo_pattern_destroy (pattern);
@@ -742,6 +748,9 @@ cairo_set_source_surface (cairo_t *cr,
if (cr->status)
return;
+ /* push the current pattern to the freed lists */
+ cairo_set_source (cr, (cairo_pattern_t *) &cairo_pattern_none);
+
pattern = cairo_pattern_create_for_surface (surface);
cairo_matrix_init_translate (&matrix, -x, -y);
diff --git a/src/cairoint.h b/src/cairoint.h
index 600eef5..0c0c37c 100644
--- a/src/cairoint.h
+++ b/src/cairoint.h
@@ -1099,6 +1099,7 @@ typedef struct _cairo_solid_pattern {
} cairo_solid_pattern_t;
extern const cairo_private cairo_solid_pattern_t cairo_pattern_nil;
+extern const cairo_private cairo_solid_pattern_t cairo_pattern_none;
typedef struct _cairo_surface_pattern {
cairo_pattern_t base;
--
1.4.4.2
More information about the cairo
mailing list