Hey Behdad,
I figured out why the dashboard figures make it look as though we're
surprisingly slow when running the XP fonts with simple scripts (e.g.
Latin, Cyrillic).
The problem is that my test framework passes "--features kern=0" for
these tests in order to suppress use of the legacy 'kern' table, so that
we'll better match Uniscribe's shaping results. But (somewhat
surprisingly, at first glance) running with kern=0 is substantially
*slower* than running without it:
$ time hb-shape arial-winxp.ttf --text-file ru.txt > /dev/null
real 0m38.831s
user 0m38.726s
sys 0m0.106s
$ time hb-shape arial-winxp.ttf --text-file ru.txt --features kern=0 >
/dev/null
real 0m50.087s
user 0m49.984s
sys 0m0.104s
Explicitly specifying kern=1 is slower still, although the shaped
results will be identical to the no-user-feature case:
$ time hb-shape arial-winxp.ttf --text-file ru.txt --features kern=1 >
/dev/null
real 0m56.122s
user 0m56.022s
sys 0m0.101s
The reason for this, as you no doubt realized right away, is that
passing *any* user features will prevent hb_shape() taking advantage of
a cached shape plan in the font, and so we re-create the plan for every
string. This is pretty expensive, and results in the slowdown here.
Caching plans with arbitrary user features may be a bit tricky, but what
I suggest we could do to address this for the common use case is to
cache plans with user features *provided* all the user features are
"global" (i.e. they have start=0, end=-1). And only use a cached plan if
the list of features is exactly the same - i.e. the same tags and values
(and global ranges) and listed in the same order. Make no attempt to
decide whether different feature lists could in fact share the same
plan, just refuse to use a cached plan if there's *any* difference. This
makes it reasonably easy and cheap to do the caching, and in practice
it's still likely to hit the vast majority of the use cases.
The attached patch implements this, and with this applied, I now see
kern=0 resulting in a substantial speed-up, as expected, instead of a
slow-down compared to the default shaping:
$ time hb-shape arial-winxp.ttf --text-file ru.txt --features kern=0 >
/dev/null
real 0m30.807s
user 0m30.722s
sys 0m0.086s
And explicitly setting kern=1 shows no significant difference from the
original no-features case (the variation from the first run above is
within the noise level):
$ time hb-shape arial-winxp.ttf --text-file ru.txt --features kern=1 >
/dev/null
real 0m37.544s
user 0m37.453s
sys 0m0.091s
So I'd suggest it's worth doing something like this - unless of course
you want to go the whole way and implement "smart" plan caching with
user features, but IMO that sounds like it might be more effort than
it's worth.
JK
commit fd387acc173e128101f2e556e44927cebd18a010
Author: Jonathan Kew <[email protected]>
Date: Tue Oct 29 19:15:57 2013 +0000
cache shape plans even if (global) user features are set
diff --git a/src/hb-shape-plan-private.hh b/src/hb-shape-plan-private.hh
index dd014e3..ee48767 100644
--- a/src/hb-shape-plan-private.hh
+++ b/src/hb-shape-plan-private.hh
@@ -45,6 +45,9 @@ struct hb_shape_plan_t
hb_shape_func_t *shaper_func;
const char *shaper_name;
+ hb_feature_t *user_features;
+ unsigned int num_user_features;
+
struct hb_shaper_data_t shaper_data;
};
diff --git a/src/hb-shape-plan.cc b/src/hb-shape-plan.cc
index b44a9e2..dcbe611 100644
--- a/src/hb-shape-plan.cc
+++ b/src/hb-shape-plan.cc
@@ -107,18 +107,27 @@ hb_shape_plan_create (hb_face_t *face,
assert (props->direction != HB_DIRECTION_INVALID);
hb_shape_plan_t *shape_plan;
+ hb_feature_t *features = NULL;
if (unlikely (!face))
face = hb_face_get_empty ();
if (unlikely (!props || hb_object_is_inert (face)))
return hb_shape_plan_get_empty ();
- if (!(shape_plan = hb_object_create<hb_shape_plan_t> ()))
+ if (num_user_features && !(features = (hb_feature_t *) malloc (num_user_features * sizeof (hb_feature_t))))
return hb_shape_plan_get_empty ();
+ if (!(shape_plan = hb_object_create<hb_shape_plan_t> ())) {
+ free (features);
+ return hb_shape_plan_get_empty ();
+ }
hb_face_make_immutable (face);
shape_plan->default_shaper_list = shaper_list == NULL;
shape_plan->face = hb_face_reference (face);
shape_plan->props = *props;
+ shape_plan->num_user_features = num_user_features;
+ shape_plan->user_features = features;
+ if (num_user_features)
+ memcpy (features, user_features, num_user_features * sizeof (hb_feature_t));
hb_shape_plan_plan (shape_plan, user_features, num_user_features, shaper_list);
@@ -147,6 +156,9 @@ hb_shape_plan_get_empty (void)
NULL, /* shaper_func */
NULL, /* shaper_name */
+ NULL, /* user_features */
+ 0, /* num_user_featurs */
+
{
#define HB_SHAPER_IMPLEMENT(shaper) HB_SHAPER_DATA_INVALID,
#include "hb-shaper-list.hh"
@@ -191,6 +203,7 @@ hb_shape_plan_destroy (hb_shape_plan_t *shape_plan)
#undef HB_SHAPER_IMPLEMENT
hb_face_destroy (shape_plan->face);
+ free (shape_plan->user_features);
free (shape_plan);
}
@@ -301,23 +314,55 @@ hb_shape_plan_hash (const hb_shape_plan_t *shape_plan)
}
#endif
-/* TODO no user-feature caching for now. */
+/* User-feature caching is currently somewhat dumb:
+ * it only finds matches where the feature array is identical,
+ * not cases where the feature lists would be compatible for plan purposes
+ * but have different ranges, for example.
+ */
struct hb_shape_plan_proposal_t
{
const hb_segment_properties_t props;
const char * const *shaper_list;
+ const hb_feature_t *user_features;
+ unsigned int num_user_features;
hb_shape_func_t *shaper_func;
};
+static inline hb_bool_t
+hb_shape_plan_user_features_match (const hb_shape_plan_t *shape_plan,
+ const hb_shape_plan_proposal_t *proposal)
+{
+ if (proposal->num_user_features != shape_plan->num_user_features) return false;
+ for (unsigned int i = 0, n = proposal->num_user_features; i < n; i++)
+ if (proposal->user_features[i].tag != shape_plan->user_features[i].tag ||
+ proposal->user_features[i].value != shape_plan->user_features[i].value ||
+ proposal->user_features[i].start != shape_plan->user_features[i].start ||
+ proposal->user_features[i].end != shape_plan->user_features[i].end) return false;
+ return true;
+}
+
static hb_bool_t
hb_shape_plan_matches (const hb_shape_plan_t *shape_plan,
const hb_shape_plan_proposal_t *proposal)
{
return hb_segment_properties_equal (&shape_plan->props, &proposal->props) &&
+ hb_shape_plan_user_features_match (shape_plan, proposal) &&
((shape_plan->default_shaper_list && proposal->shaper_list == NULL) ||
(shape_plan->shaper_func == proposal->shaper_func));
}
+static inline hb_bool_t
+hb_non_global_user_features_present (const hb_feature_t *user_features,
+ unsigned int num_user_features)
+{
+ while (num_user_features)
+ if (user_features->start != 0 || user_features->end != (unsigned int) -1)
+ return true;
+ else
+ num_user_features--, user_features++;
+ return false;
+}
+
/**
* hb_shape_plan_create_cached:
* @face:
@@ -339,12 +384,11 @@ hb_shape_plan_create_cached (hb_face_t *face,
unsigned int num_user_features,
const char * const *shaper_list)
{
- if (num_user_features)
- return hb_shape_plan_create (face, props, user_features, num_user_features, shaper_list);
-
hb_shape_plan_proposal_t proposal = {
*props,
shaper_list,
+ user_features,
+ num_user_features,
NULL
};
@@ -382,6 +426,11 @@ retry:
hb_shape_plan_t *shape_plan = hb_shape_plan_create (face, props, user_features, num_user_features, shaper_list);
+ /* Don't add the plan to the cache if there were user features with non-global ranges */
+
+ if (hb_non_global_user_features_present (user_features, num_user_features))
+ return shape_plan;
+
hb_face_t::plan_node_t *node = (hb_face_t::plan_node_t *) calloc (1, sizeof (hb_face_t::plan_node_t));
if (unlikely (!node))
return shape_plan;
_______________________________________________
HarfBuzz mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/harfbuzz
