Re: Slow application startup when using a theme

[Fred Kiefer]

On 10.01.2012 05:54, Eric Wasylishen wrote:
> On 2012-01-09, at 2:26 PM, Fred Kiefer wrote:
> > > Much of the overhead when using Neos is caused by the section of
> > > -[GSTheme activate] which loads all images in the theme.
> > >
> > > Here are some tests: startup Ink with default theme: 51k
> > > syscalls startup Ink with Neos.theme: 183k syscalls startup Ink
> > > with Neos.theme, with GSTheme.m lines 473 to 539 commented out:
> > > 95k syscalls.
> >
> > I am not completely sure why this code is that way. What I remember
> > is that we need to override all the already loaded images. But
> > there was more to it. It could have to do with unloading an image
> > and restoring the previous image state. I hate the way we deal with
> > images in themes and have often stated this, but haven't come up
> > with a better idea up to now.
>
> I didn't like the current approach either…  I just committed a
> rewrite of how theme images are handled which gets rid of theme
> proxies entirely, and eliminates the code in -[GSTheme activate] and
> -[GSTheme deactivate] which were setting/restoring images.
>
> Now, when a name is set on an NSImage instance, it also subscribes to
> the theme change notification. When an NSImage receives this
> notification, it does the name-to-path lookup again that +[NSImage
> imageNamed:] did, and reloads itself (discarding all reps) if the
> path has changed.
>
> It's a fairly major change but I tested Gorm, GSTest, SimpleAgenda,
> and tried switching between the GNUstep default theme, Neos, and
> Etoile's Nesedah theme while the apps were running, and all images
> seemed to update properly.

I am completely impressed by what you did here. On the other hand, I 
cannot look at code without trying to find flaws in it. Sorry, I am just 
made that way.

So this is what I found:

- After your code is verified by tests we should get rid of the complete 
class GSThemeProxy.

- The code in [NSImage imageNamed:] could be simpified, not that the 
proxy is gone. We don't need to get the same image we just created from 
the dictionary.

- I don't quite understand what happens when we switch back from having 
a theme to not using one. Will the standard GNUstep images get reused? 
The question here is whether we send the notification 
GSThemeDidActivateNotification when there is no theme at all.

- When the old theme included an image, but the new one doesn't we keep 
the old image. Is this what we want? Surely it is better than having no 
image.

- Should we really reset the size in themeDidActivate: when 
sizeWasExplicitlySet is YES?

- The method pathForImageNamed: should go into the private category and 
have an underscore.

- Why is the method pathForImageNamed: using the imageLock? I think this 
lock should only be used to protect the name and the name dictionary.

- Images that were not loaded from a file, but have a name assigned to 
them would be a problem. A theme could create an image on the fly and 
assign a standard name to it. I think the best solution here is just to 
not allow this behaviour :-)

Looking at this small list I must say, your code is almost perfect.