Hi
I include a patch file for some of the things discussed below. I noticed a couple
have been changed in the past few days as it is.
Dave
Dave Trollope wrote:
> Hi Dominik,
>
> > Fvwm's speed depends to 90% or more on how the communication with
> > the X server is done. All other effects of calling functions and
> > such only add very little to the CPU load. Once I profiled fvwm
> > for ten minutes using gprof, and as far as I remember it did not
> > even consume one second of CPU time. The things that slow it down
> > are file I/O and X server communication.
>
> I agree there are bigger fish to fry - this was my first look at fvwm source.
> I thought it wiser to start with small steps.... I was thinking I would take a
> look at XFree86 too sometime. I need to learn much about X primitives.
>
> In terms of the performance - I felt like the system was more responsive in
> cygwin with the modifications. Perhaps it was just my hopeful imagination!
> What OS and CPU were you running when you profiled?
>
> > Talking about the code execution time, there is only one big
> > factor, the PositiveWrite() function that sends information to
> > the modules. It can be called hundreds of thousands of times
> > during a busy work day. Much effort has been made to optimize
> > that function.
> >
>
> AddToMessageQueue has 2 safemalloc calls - That equates to "hundreds of
> thousands"*2 unnecessary function calls.
>
> If you want to really optimise this, the first call could use malloc directly
> dispensing with an unnecessary if (length == 0) as well as the function call.
> Shouldn't do this on the second one because size could be any value.
>
> > > 1) I came across libs/safemalloc.c.
> > >
> > > I was surprised that malloc was wrapped in a function like this since
> > > this is called whenever something is put on the (I think) event queue.
> > > Everytime I move the pointer from one window to another, launch tools
> > > etc, safemalloc is called. Wrapping malloc in a function is an overhead
> > > that will cause branches from the CPU cache probably when only one is
> > > necessary (the actual call to malloc itself).
> >
> > I feel the added code size is not worth the effort most of the
> > time. If individual functions are called ofthen from a certain
> > place it may be better to only optimize that place instead of all
> > the others where it isn't a problem. For example, many of the
> > safemalloc() calls could be replaced with alloca(), even saving
> > the free() call.
>
> Hmm that sounds like a good change to do. I will look at that.
>
> > > So #if 0'd out the safemalloc, safecalloc, saferealloc functions and
> > > recoded them as macros. E.G.
> > >
> > > #define safemalloc(length) \
> > > (malloc((length) <= 0 ? 1 : (length)) ? : (char
> > > *)alloc_failed("malloc",(length)))
> > >
> > > The main advantage of this is that only one function call for each
> > > malloc is required (malloc itself) instead of two (safemalloc+malloc).
> > > ?: is supposed to be more efficient than if () but I'm not sure thats
> > > really true. If I could only disassemble the code.... Suggestions? One
> > > function call should reduce the number of branches (and therefore cache
> > > usage is improved).
> > >
> > > The biggest disadvantage of this however is that the binary size
> > > increases because these decisions (length <= 0) are duplicated
> > > everywhere. Somewhat prone to casting issues of length.
> >
> > > 2) The other related issue I found was that in some cases, dynamic
> > > memory is used for simple temporary use. Local stack arrays are much
> > > more efficient for this, however you don't want to restrict the size of
> > > the data being processed. I suggest that the majority of users will have
> > > little data and most optimisation can come from putting those into a
> > > local array, and only calling on safemalloc when handling larger data
> > > quantities.
> > >
> > > [ code snipped ]
> > > This would completely remove the malloc for 90% of users.
> >
> > Yes, but what is the cost of the malloc?
>
> What does the cost matter when it can be reduced to 10% of the cost?
>
> > RelieveRectangle is
> > perhaps called 20 times when a window changes focus. I'd guess on
> > average you do not do this more than 5 times per minute, giving a
> > total of 6000 calls per hour. I'd estimate that can easily be
> > done by any but the slowest machines in less than the tenth of a
> > second.
>
> My only contention here is that if you don't need to do it, why do it. Even
> tenths of seconds here and there add up.
>
> > It is likely that more time could be saved by preventing
> > unnecessary calls to RelieveRectangle() in a single place in the
> > code since it requires communication over the network.
>
> Sounds like another promising avenue - I will take a look.
>
> > > I have half a dozen of these kind of things and a place or two where
> > > memset is called prior to a memcpy that overwrites the same block memset
> > > cleared.
> >
> > Do you have a list of these? Some of the memset() calls are
> > redundant. Not all of these are bad though. Some are merely
> > intended to prevent uninitialised elements of structures if new
> > members are added to the structures in the future.
>
> The places where I'm thinking of use sizeof in both calls so if one misses a
> new element, so would the other.
>
> Here they are:
>
> menus.c NewMenuStyle()
> tmpms = (MenuStyle *)safemalloc(sizeof(MenuStyle));
> memset(tmpms, 0, sizeof(MenuStyle));
> if (ms)
> {
> /* copy the structure over our temporary menu face. */
> memcpy(tmpms, ms, sizeof(MenuStyle));
>
> The memset can be safely moved to the else leg of the if.
>
> scanForColor()
> /* save instring in case can't find pixmap */
> save_instring = (char *)safemalloc(strlen(instring)+1);
> name = (char *)safemalloc(strlen(instring)+1);
> strcpy(save_instring,instring);
>
> /* Scan whole string */
> for (txt = instring; *txt != '\0'; txt++)
> {
> ...
> }
> free(name);
> free(save_instring);
>
> can become:
> int l = strlen(instring) + 1;
> /* save instring in case can't find pixmap */
> save_instring = (char *)safemalloc(l*2);
> name = save_instring + l;
> strcpy(save_instring,instring);
>
> /* Scan whole string */
> for (txt = instring; *txt != '\0'; txt++)
> {
> ...
> }
> free(save_instring);
>
> add_window.c setup_window_structure()
>
> This memset isn't needed its only used in the else leg, and then its copied
> over.:
>
> memset(&save_state, '\0', sizeof(FvwmWindow));
>
> I thought I found more, but these were the ones I had written down.
>
> > > What do you think? While these are trivial changes, for a function that
> > > is called hundreds of times, even a small gain adds up. Shall I go ahead
> > > and make these changes? Perhaps as I grow more experienced with the
> > > fvwm2 source I can find some bigger issues.
> >
> > I suggest this:
> >
> > - Find places where alloca() can be used instead of safemalloc.
> > alloca is either a system function or a replacement from the
> > fvwm library. This eliminates the additional function call and
> > even cuts down the chances of running out of memory. A program
> > that can't get more space on the stack is doomed to crash
> > anyway. It also reduces the chance of memory leaks.
> > - Leave safemalloc as it is. If there are specific functions
> > that are called frequently, optimizing them is a good thing.
> > - In many places there is code like
> > ptr = safemalloc(size);
> > memset(ptr, 0, size);
> > This could be done faster and smaller with safecalloc().
> > - For the big gain: optimize the X calls. Any call that
> > contacts the X server needlessly causes delays.
> >
>
> I can easily attack the first three. My current inexperience with X limits the
> last one, but I am interested in learning X development so I will look around
> (I will look at the ReleiveRectangle calls first).
>
> > And a comment on coding style:
> >
> > - Macros with code are evil. Never use them. Instead, you can
> > inline the code (although it doesn't work with any compiler).
>
> Here we will have to agree to disagree. I believe macros with and without code
> have their place. There are certain things that shouldn't be done in macros,
> but I wouldn't call them evil!
>
> Cheers
> Dave
>
> >
> > Bye
> >
> > Dominik ^_^ ^_^
> >
> > --
> > Dominik Vogt, email: d.vogt_at_lifebits.de
> > LifeBits Aktiengesellschaft, Albrechtstr. 9, D-72072 Tuebingen
> > fon: ++49 (0) 7071/7965-0, fax: ++49 (0) 7071/7965-20
? origexes
? alloc.pch
Index: fvwm/add_window.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/add_window.c,v
retrieving revision 1.282
diff -u -u -r1.282 add_window.c
--- fvwm/add_window.c 2002/03/25 00:09:35 1.282
+++ fvwm/add_window.c 2002/03/25 06:10:10
_at_@ -376,8 +376,6 @@
FvwmWindow save_state;
FvwmWindow *savewin = NULL;
- memset(&save_state, '\0', sizeof(FvwmWindow));
-
/*
Allocate space for the FvwmWindow struct, or reuse an
old one (on Recapture).
_at_@ -385,10 +383,6 @@
if (ReuseWin == NULL)
{
*pfw = (FvwmWindow *)safemalloc(sizeof(FvwmWindow));
- if (*pfw == (FvwmWindow *)0)
- {
- return False;
- }
}
else
{
Index: fvwm/menustyle.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/fvwm/menustyle.c,v
retrieving revision 1.2
diff -u -u -r1.2 menustyle.c
--- fvwm/menustyle.c 2002/03/23 11:59:59 1.2
+++ fvwm/menustyle.c 2002/03/25 06:10:15
_at_@ -270,7 +270,7 @@
}
else
{
- buffer = (char *)safemalloc(strlen(action) + 100);
+ buffer = (char *)alloca(strlen(action) + 100);
sprintf(buffer,
"* \"%s\", Foreground \"%s\", Background \"%s\", "
"Greyed \"%s\", Font \"%s\", \"%s\"",
_at_@ -279,7 +279,6 @@
"Animation" : "AnimationOff");
action = buffer;
menustyle_parse_style(F_PASS_ARGS);
- free(buffer);
}
if (fore)
Index: libs/Graphics.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/libs/Graphics.c,v
retrieving revision 1.55
diff -u -u -r1.55 Graphics.c
--- libs/Graphics.c 2002/03/23 01:18:15 1.55
+++ libs/Graphics.c 2002/03/25 06:10:19
_at_@ -72,7 +72,7 @@
}
return;
}
- seg = (XSegment*)safemalloc(sizeof(XSegment) * line_width);
+ seg = (XSegment*)alloca(sizeof(XSegment) * line_width);
/* left side, from 0 to the lesser of line_width & just over half w */
for (i = 0; (i < line_width) && (i <= w / 2); i++) {
seg[i].x1 = x+i; seg[i].y1 = y+i+a;
_at_@ -97,7 +97,6 @@
seg[i].x2 = x+i+1-a; seg[i].y2 = y+i;
}
XDrawSegments(dpy, d, ReliefGC, seg, i);
- free(seg);
return;
}
Index: libs/Module.c
===================================================================
RCS file: /home/cvs/fvwm/fvwm/libs/Module.c,v
retrieving revision 1.31
diff -u -u -r1.31 Module.c
--- libs/Module.c 2002/03/04 07:26:00 1.31
+++ libs/Module.c 2002/03/25 06:10:23
_at_@ -164,14 +164,13 @@
while ( (temp = strchr(hold, ',')) != NULL )
{
- char *temp_msg = (char*)safemalloc(temp - hold + 1);
+ char *temp_msg = (char*)alloca(temp - hold + 1);
strncpy(temp_msg, hold, (temp - hold));
temp_msg[(temp - hold)] = '\0';
hold = temp + 1;
SendText(fd, temp_msg, window);
- free(temp_msg);
} /* while */
/*
--
Visit the official FVWM web page at <URL: http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm" in the body of a
message to majordomo_at_fvwm.org.
To report problems, send mail to fvwm-owner_at_fvwm.org.
Received on Mon Mar 25 2002 - 00:17:08 GMT