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 377495 - Consolidate core.h calls to save lookups
Consolidate core.h calls to save lookups
Status: RESOLVED FIXED
Product: metacity
Classification: Other
Component: general
trunk
Other Linux
: Normal normal
: ---
Assigned To: Thomas Thurman
Metacity maintainers list
Depends on:
Blocks:
 
 
Reported: 2006-11-20 19:42 UTC by Thomas Thurman
Modified: 2007-06-07 03:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Refactor to save lookups (25.81 KB, patch)
2006-11-20 19:54 UTC, Thomas Thurman
needs-work Details | Review

Description Thomas Thurman 2006-11-20 19:42:11 UTC
This is a refactor I mentioned in a FIXME a long time ago, but last night on Greyhound I finally had the time to do it. It makes frame expose events execute with 33% fewer instructions, according to callgrind.

The best explanation is probably the comment I added in the patch to core.h, describing the new meta_core_get function:

/* General information function about the given window. Pass in a sequence of
 * pairs of MetaCoreGetTypes and pointers to variables; the variables will be
 * filled with the requested values. End the list with META_CORE_GET_END.
 * For example:
 *
 *   meta_core_get (my_display, my_window,
 *     META_CORE_GET_X, &x,
 *     META_CORE_GET_Y, &y,
 *     META_CORE_GET_END);
 *
 * If the window doesn't have a frame, this will raise a meta_bug. To suppress
 * this behaviour, ask META_CORE_WINDOW_HAS_FRAME as the *first* question in
 * the list. If the window has no frame, the answer to this question will be
 * False, and anything else you asked will be undefined. Otherwise, the answer
 * will be True. The answer will necessarily be True if you ask the question
 * in any other position. The positions of all other questions don't matter.
 *
 * The reason for this function is that some parts of the program don't know
 * about MetaWindows. But they *can* see core.h. So we used to have a whole
 * load of functions which took a display and an X window, looked up the
 * relevant MetaWindow, and returned information about it. The trouble with
 * that is that looking up the MetaWindow is a nontrivial operation, and
 * consolidating the calls in this way makes (for example) frame exposes
 * 33% faster, according to valgrind.
 *
 * This function would perhaps be slightly better if the questions were
 * represented by pointers, perhaps gchar*s, because then we could take
 * advantage of gcc's automatic sentinel checking. On the other hand, this
 * immediately suggests string comparison, and that's slow.
 *
 * Another possible improvement is that core.h still has a bunch of
 * functions which can't be described by the formula "give a display and
 * an X window, get a single value" (meta_core_user_move, for example), but
 * which could theoretically be handled by this function if we relaxed the
 * requirement that all questions should have exactly one argument.
 */
Comment 1 Thomas Thurman 2006-11-20 19:54:10 UTC
Created attachment 76937 [details] [review]
Refactor to save lookups

What do people think?
Comment 2 Elijah Newren 2007-04-03 17:48:53 UTC
I'm not opposed, and the 33% speedup for frame exposes sounds nice.  Would be nice to have Havoc comment since I tend to avoid the frame and theme code, and sometimes by extension the core.c code.  If he doesn't comment soon, feel free to update to trunk, fix my nitpicks below and commit.

A couple minor nitpicks, all about spacing:

Arguments should align.  Rather than:
 +meta_core_get (Display *xdisplay,
 +    Window xwindow,
 +    ...)
you should have
 +meta_core_get (Display *xdisplay,
 +               Window xwindow,
 +               ...)

Also, rather than
 +  meta_core_get (gdk_display, frame->xwindow,
 +      META_CORE_GET_CLIENT_WIDTH, &width,
 +      META_CORE_GET_CLIENT_HEIGHT, &height,
 +      META_CORE_GET_FRAME_FLAGS, &flags,
 +      META_CORE_GET_FRAME_TYPE, &type,
 +      META_CORE_GET_END
 +      );
you should have
 +  meta_core_get (gdk_display, frame->xwindow,
 +                 META_CORE_GET_CLIENT_WIDTH, &width,
 +                 META_CORE_GET_CLIENT_HEIGHT, &height,
 +                 META_CORE_GET_FRAME_FLAGS, &flags,
 +                 META_CORE_GET_FRAME_TYPE, &type,
 +                 META_CORE_GET_END
 +                 );

There should be spaces between operators.  Rather than
 window!=NULL && window->frame!=NULL;
it should be
 window != NULL && window->frame != NULL;

(Go above the call of duty; help clean up the code:) Kill ugly tabs.  If you are changing a line of code with a tab in it anyway, please remove the tab.  If you are changing a line adjacent to one with one of those nasty characters, you may as well gratuitously remove it anyway (it won't prevent patches working against multiple versions of metacity any more than they'll already be broken given your nearby changes).  e.g. Please change
 -               meta_core_get_frame_type (gdk_display, frame->xwindow),
 +               frame_type,
		frame->text_height,
 -		meta_core_get_frame_flags (gdk_display, frame->xwindow),
 +		frame_flags,
to
 -               meta_core_get_frame_type (gdk_display, frame->xwindow),
 -		frame->text_height,
 -		meta_core_get_frame_flags (gdk_display, frame->xwindow),
 +               frame_type,
 +               frame->text_height,
 +               frame_flags,

Thanks, as always.
Comment 3 Elijah Newren 2007-04-03 18:00:24 UTC
(Getting the patch off the radar; not sure what to select, I guess needs-work due to the spacing issues and because I'd like Havoc to comment, but accepted is almost as right)
Comment 4 Havoc Pennington 2007-04-03 18:12:13 UTC
Looks fine to me (do fix the whitespace, in addition to tabs I saw a "!=" with no spaces around it)
Comment 5 Thomas Thurman 2007-06-07 03:32:21 UTC
Okay, brought up to date, amended, and committed. Metacity handles noticeably faster, unless it's my imagination. (This is a slow computer, though.)