On Fri, Mar 22, 2002 at 09:30:10PM -0600, 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?
Linux 2.2.something on a 400MHz Pentium 2.
> > 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.
Yeah, that's rather inefficient. You can also merge
AddToMessageQueue into PositiveWrite and replace the two calls
to safemalloc with one to malloc. I'll commit a patch.
[snip]
> > > 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?
I wanted to point out that the cost is low compared to other code.
> > 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.
But on the other hand, the additional if's and the duplicate code
enlarge the binary and make the code harder to maintain.
> > > 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.
Yup, I'll do that.
> 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);
I'll use alloca instead and get rid of the free() alltogether.
> 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.
I appreciate that work. There are a lot of small enhancements
that can be made. If you want to continue, please download the
latest snapshot or - better - the code from cvs (instructions for
cvs are on our home page). Large parts of the code have changed
since 2.4.x and it can become tedious to port these patches to the
development branch. Also, it's much easier to apply paches that
were created with diff: Make a copy of the files you want to
change, edit them and run diff:
$ cp foo.c foo.c.orig
(edit foo.c)
$ diff -u foo.c.orig foo.c > foo.patch
These patches can then be applied automatically with the patch
program. (If the -u option is not supported by your diff version,
use -c instead).
> > > 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).
Basically the situation is this: When X...() calls are made, the
commands are usually queued on the client until they are sent to
the server with XFlush or XSync is called. This involved packets
going over the network - which is slower than the rest of the code
by order of magnitudes, even if the X server is on the same
machine. So one goal would be to minimize the number of XFlush
and XSync calls so that you get a few big packets instead of many
small ones. Since XSync waits for the server to process all
commands, it is slower than XFlush, but sometimes it is necessary
to do this.
It gets even more complicated because a lot of X...() calls (e.g.
all XGet... and XQuery... functions) automatically flush the so
called request buffer. Reducing the number of such calls is
another goal for optimisation. Unfortunately I could never find a
complete list of the X functions that flush the request buffer.
> > 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!
Okay, I sometimes put code in macros too, but *only* if it is a
single command like
#define SET_USER_STATES(fw, mask) (fw)->flags.user_states |= (mask)
Longer macros almost always lead to tears at some point in the
future. Sure, functions are inefficient compared with macros in
terms of speed, but with that reasoning you'd have to put the
whole program in the main function. I bet you could even make it
smaller with the 'proper' use of gotos ;-)
Bye
Dominik ^_^ ^_^
--
Dominik Vogt, dominik.vogt_at_gmx.de
Reply-To: dominik.vogt_at_gmx.de
--
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 Sat Mar 23 2002 - 06:01:15 GMT