After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 322174 - Don't use G_TYPE_INSTANCE_GET_PRIVATE in PangoFcFont
Don't use G_TYPE_INSTANCE_GET_PRIVATE in PangoFcFont
Status: RESOLVED FIXED
Product: pango
Classification: Platform
Component: general
1.10.x
Other Linux
: Normal normal
: ---
Assigned To: pango-maint
pango-maint
Depends on:
Blocks:
 
 
Reported: 2005-11-22 20:40 UTC by Federico Mena Quintero
Modified: 2005-11-22 22:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Profile for pango_fc_font_get_glyph() (27.92 KB, image/png)
2005-11-22 20:41 UTC, Federico Mena Quintero
  Details
Profile after optimization. Notice how the time went from 8.37% to 1.31% (7.66 KB, image/png)
2005-11-22 20:43 UTC, Federico Mena Quintero
  Details
pango-fcfont-no-gobject-private-data.diff (5.97 KB, patch)
2005-11-22 20:44 UTC, Federico Mena Quintero
committed Details | Review

Description Federico Mena Quintero 2005-11-22 20:40:40 UTC
The attached screenshots from sysprof show two things:

1. PangoFcFont uses G_TYPE_INSTANCE_GET_PRIVATE().  In particular, it gets used
in pango_fc_font_get_glyph(), which is a time-critical function.  We spend 8.37%
of the pango-language-benchmark time in that function, and 3.35% is getting the
private data.

2. pango_fc_font_get_glyph() has a type check in a g_return_val_if_fail(); that
one takes 2.13% of the total time.

To fix (1), I looked for a field to replace with a "priv" in the PangoFcFont
structure.  That structure is private, so I can't change any field.  There's a
"gpointer context_key" field, used only internally, so I replaced it with
"gpointer priv".  I added internal accessors for the context_key, and used them
elsewhere in Pango.

The fix for (2) is trivial; just take out that g_return_val_if_fail().
Comment 1 Federico Mena Quintero 2005-11-22 20:41:57 UTC
Created attachment 55106 [details]
Profile for pango_fc_font_get_glyph()
Comment 2 Federico Mena Quintero 2005-11-22 20:43:09 UTC
Created attachment 55107 [details]
Profile after optimization.  Notice how the time went from 8.37% to 1.31%
Comment 3 Federico Mena Quintero 2005-11-22 20:44:01 UTC
Created attachment 55108 [details] [review]
pango-fcfont-no-gobject-private-data.diff
Comment 4 Federico Mena Quintero 2005-11-22 21:42:21 UTC
Ahem, PangoFcFont is *public*, so I can't change its size.  Sorry for the thinko
above.
Comment 5 Behdad Esfahbod 2005-11-22 21:44:40 UTC
In HEAD now.

2005-11-22  Federico Mena Quintero  <federico@ximian.com>

        Fixes #322174:

        * pango/pangofc-font.h (struct _PangoFcFont): Replace the
        "gpointer context_key" field with "gpointer priv".  This way we
        can access the private data quickly, instead of using
        g_type_instance_get_private().

        * pango/pangofc-private.h: Added prototypes for
        _pango_fc_font_{get,set}_context_key().

        * pango/pangofc-font.c (struct _PangoFcFontPrivate): Moved the
        "context_key" field to here.
        (PANGO_FC_FONT_GET_PRIVATE): Use the "priv" field instead of GType
        private data.
        (pango_fc_font_class_init): Don't register GType private data.
        (pango_fc_font_init): Initialize the private data here.
        (pango_fc_font_finalize): Free the private data.
        (_pango_fc_font_get_context_key): Implement.
        (_pango_fc_font_set_context_key): Implement.
        (pango_fc_font_get_glyph): Remove the g_return_val_if_fail(); it
        was appearing quite high in the profile.

        * pango/pangofc-fontmap.c (pango_fc_font_map_add): Call
        _pango_fc_font_set_context_key() instead of setting the
        fcfont->context_key directly.
        (_pango_fc_font_map_remove): Likewise; also use
        _pango_fc_font_get_context_key() instead of accessing the field
        directly.

Comment 6 Christian Persch 2005-11-22 21:59:31 UTC
-  g_type_class_add_private (object_class, sizeof (PangoFcFontPrivate));
[...]
+  priv = g_new0 (PangoFcFontPrivate, 1);
+  fcfont->priv = priv;

That looks wrong to me; just because you don't want to use
G_TYPE_INSTANCE_GET_PRIVATE everywhere to get the priv pointer doesn't mean you
cannot use g_type_class_add_private... ; you just use
G_TYPE_INSTANCE_GET_PRIVATE in pango_fc_font_init to set the instance->priv
pointer and access instance->priv directly elsewhere.
Comment 7 Federico Mena Quintero 2005-11-22 22:11:06 UTC
g_type_class_add_private() tells the type system to allocate the private
structure somewhere.

Then, in ::init(), I would have to get a pointer to the structure and stuff it
in my ->priv.

By doing it with g_new(), I have less code, and less places that deal with
creating the private data.  This all makes the code clearer.
Comment 8 Behdad Esfahbod 2005-11-22 22:25:19 UTC
Good point Christian.  That's how it's done in other places in Pango too.

2005-11-22  Behdad Esfahbod  <behdad@gnome.org>

        * pango/pangofc-font.c: Finish previous patch.  Use GType private
        data, cache it into fcfont->priv.  Get rid of
        PANGO_FC_FONT_GET_PRIVATE (fcfont) and access fcfont->priv. (#322174,
        Christian Persch)