79132 - Xlib toolkit is kinda inefficient

Status: RESOLVED FIXED

(Core Graveyard :: Tracking, defect)

Description

[timeless]

minor bug based on response to the 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=33348 to bug 78548.

static void FreeWeights(nsFontWeight* aWeight)

static void FreeStyle(nsFontStyle* aStyle)

use ++i and
cache pointer dereferences.

Comment 1

[timeless]

Attachment: changes

Comment 2

[timeless]

patch; seeking review+approval.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

r=dbaron.  Is this in the GTK toolkit as well?

Comment 4

[timeless]

quite possibly, since these toolkits really copy/paste the code.  But one 
toolkit at a time :).

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/src/gtk/nsFontMetricsGT
K.cpp&rev=1.137&mark=520-550#515

conveniently, the functions don't have the same name, so it didn't turn up w/ 
an lxr ident search (i wonder if that's a requirement, if so why don't these 
calls have toolkit name mangling?)

Looking for an sr= for the xlib version, and I'll get a patch for the gtk 
version (and i'll check qt ...)

Oddly, gtk's ver is 1..9 for both, whereas xlib is 1..9 and 1..3.  I haven't 
tried to look at the fundamentals so I don't know why this would be the case.

Comment 5

[Alec Flett]

boy, someone's been paying too much attention in his computer architecture
class, and not enough attention in his compiler class..

unless this is a tight loop, this really isn't affecting much, and the switch
from i++ to ++i likely makes no difference in the compiled code... but the patch
can do nothing but good, so sr=alecf... but your efforts could be MUCH better
utilized elsewhere

Comment 6

[Brendan Eich [:brendan]]

timeless: do not mess with any i++ in my code where the value is discarded -- it
is just as fast in cycles, and dense in code space, as ++i (any compiler worth a
damn will notice the unused result and avoid the temporary).

Avoiding reloads due to ambiguous memory, OTOH, is something worth doing in some
tight loops, or as a matter of style if you like.  In loops that are not on any
performance critical path, doing so still reduces code bloat (although it can
harm readability by introducing explicit temporaries) so on balance I'm open to
it.  But really, there are many, many more important tasks before us in the way
of mozilla1.0.  Check your priorities please.

/be

Comment 7

[timeless]

Attachment: gtk parity

Comment 8

[timeless]

xlib fix checked in. if people want to provide r=/sr= for gtk they can.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you compile before checking in?  I needed the following fix to compile xlib
(which I'm about to check in):

Index: nsFontMetricsXlib.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp,v
retrieving revision 1.46
diff -u -d -r1.46 nsFontMetricsXlib.cpp
--- nsFontMetricsXlib.cpp	2001/05/08 09:34:17	1.46
+++ nsFontMetricsXlib.cpp	2001/05/08 22:12:41
@@ -517,7 +517,7 @@
 static void
 FreeWeights(nsFontWeight* aWeight)
 {
-  nsFontStretch* stretches=aWeight->mStretches;
+  nsFontStretch** stretches=aWeight->mStretches;
   for (int i=0; i < 9; i++)
   {
     if (stretches[i])
@@ -536,7 +536,7 @@
 static void
 FreeStyle(nsFontStyle* aStyle)
 {
-  nsFontWeight* weights=aStyle->mWeights;
+  nsFontWeight** weights=aStyle->mWeights;
   for (int i=0; i < 9; i++)
   {
     if (weights[i])

Comment 10

[timeless]

Attachment: whoops. I don't know how I lost those two *'s. I should have attached it from the box that built the patch.