104962 - 3 calls to GetWndowsFolder costs 3% of startup

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Suresh Duddi (gone)]

We call GetWindowsFolder() 3 times and this costs 3.3% of startup. Wonder what
they are for and if there are better ways of doing this.

Comment 1

[Doug Turner (:dougt)]

wow.  3%?

Do you know what the parameter of these three calls is?  Are there more than 3
calls to GetWindowsFolder but only 3 of them take the percentage that you cite?

GetWindowsFolder is a windows system call wrapper that returns well known places
on the users machine.  IIRC, we cache the result, so no two requests should ever
be the same.  Briefly looking at this code, I quickly get sick. .  Reassign and
I will try to fix it up for 9.6.

Comment 2

[Suresh Duddi (gone)]

Thanks dan. Reassigning.

I see only three calls on startup. The calls are:
Application Dir [1]
IE Favouries [2]
  - bookmarks code
  - file system data source

Comment 3

[Doug Turner (:dougt)]

(did you see how I offered to help fix this up, but totally avoided the real
work while having it assigned to my coworker.  mind control. $9.95 a month.
contact me directly :-) )

Seriously, did you mean to reassign it to me or dan?

Comment 4

[Suresh Duddi (gone)]

To you Mr. Dougt. And now to bed right away before I screwup more.

Comment 5

[Doug Turner (:dougt)]

Attachment: proposed patch v.1

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 53780 [details] [diff] [review]
proposed patch v.1

>Index: nsSpecialSystemDirectory.cpp
>===================================================================
>+    int len = PL_strlen(path);
>+    if (len>1 && path[len-1] != '\\') 
>+    {
>+        path[len]   = '\\';
>+        path[len + 1] = '\0';
>+    }

What if Windows filled up the buffer? unlikely, but you could
go over with your slash if it does.

Throw in a "&& len < sizeof path" and r=dveditz

Do we know what locations we're asking for? perhaps part of the
delay is simply firing up the shell32.dll the first time (although
by avoiding it here we might simply be moving the delay elsewhere).

If it's just the windows directory or system directory then special
casing to use GetWindowsDirectory() and GetSystemDirectory()
(which are in the kernel) might speed things up.

Comment 7

[Doug Turner (:dougt)]

your right.  the string should be allocated MAX_LEN+1

Comment 8

[Suresh Duddi (gone)]

Tested patch and it eliminated the 3% and improved startup by that amount.

Very cool dougt.

Comment 9

[Darin Fisher]

Comment on attachment 53780 [details] [diff] [review]
proposed patch v.1

sr=darin (nice find)

Comment 10

[Doug Turner (:dougt)]

Checking in nsSpecialSystemDirectory.cpp;
/cvsroot/mozilla/xpcom/io/nsSpecialSystemDirectory.cpp,v  <-- 
nsSpecialSystemDirectory.cpp
new revision: 1.46; previous revision: 1.45
done

Comment 11

[John Bandhauer]

No! SHGetSpecialFolderPath is not present in all target versions of Win32.

See the docs on the function on the MS site.

On NT4 I get a modal dialog from the system telling me that that entry point is
not found in shell32.dll

BACK IT OUT!

Comment 12

[Doug Turner (:dougt)]

crap.  you need IE4 to use this SYSTEM api.  Ill back it out.

Comment 13

[Doug Turner (:dougt)]

I will dymanically look up this symbol in the next patch.

Comment 14

[Doug Turner (:dougt)]

Backed out:
Checking in nsSpecialSystemDirectory.cpp;
/cvsroot/mozilla/xpcom/io/nsSpecialSystemDirectory.cpp,v  <--
nsSpecialSystemDirectory.cpp
new revision: 1.47; previous revision: 1.46
done

Comment 15

[nhottanscp]

The file was backed out, removing keyword 'smoketest'.

Comment 16

[Bernard Alleysson]

IE4 is not needed but shfolder.dll *is*

a search through MSDN got me here:

"
A-2. Use SHGetFolderPath To Determine Special Folder Paths
Whenever you access any of the special folders in the following list, your
application should use the Win32 APIs to dynamically obtain the proper
language-specific folder names. The preferred way to do this is using the
SHGetFolderPath API with the appropriate CSIDL constant. This function behaves
consistently across Windows 95, Windows 98, Windows NT 4.0, and Windows 2000.
This API is redistributable via the SHFOLDER.DLL. Software vendors are
encouraged to redistribute this component as much as possible to enable this
support on Windows operating systems prior to Windows 2000. Windows 2000
includes this DLL as a protected system file and, as such, this DLL cannot be
replaced on Windows 2000 or greater.

Note   To ensure your application can run on Windows 9x, Windows NT 4.0 and
Windows 2000, always link to the SHGetFolderPath implementation in SHFOLDER.DLL.
Windows 2000 natively implements SHGetFolderPath in SHELL32.DLL, but other
versions of Windows do not include SHGetFolderPath in SHELL32.DLL.
"

from:

Specifications,
Application Specification for Microsoft Windows 2000 Server,
Appendix A : Best Practices

"I will dymanically look up this symbol in the next patch"

The symbol should be looked up in shfolder.dll, not shell32.dll
So that this could work on Windows 9x and NT4 if someone decides to redistribute
shfolder.dll

just a minor comment on the source:

version 1.47:

// Get the shell's allocator.                                               
if (!SUCCEEDED(SHGetMalloc(&pMalloc)))                                      
  return;                                                                      
                                                           

// Allocate a buffer                                                        
if ((pBuffer = (LPSTR) pMalloc->Alloc(MAX_PATH + 2)) == NULL)
// XXXXXXXXXX pMalloc should be released here with pMalloc->Release();
  return;                                                    

             

Comment 17

[Suresh Duddi (gone)]

Removing from blocker list.

Comment 18

[Doug Turner (:dougt)]

Attachment: Proposed patch v.2 - dynamic loading of symbol

Comment 19

[John Bandhauer]

+#if defined (XP_WIN)
+BOOL (WINAPI * getSpecialPathProc) (HWND hwndOwner, LPSTR lpszPath, int 
nFolder, BOOL fCreate) = NULL;
+static HINSTANCE gShell32DLLInst = NULL;
+#endif
+NS_COM void StartupSpecialSystemDirectory()
+{
+#if defined (XP_WIN)
+    gShell32DLLInst = LoadLibrary("Shell32.dll");
+    if(gShell32DLLInst)
+    {
+        getSpecialPathProc  = (BOOL (WINAPI *) (HWND hwndOwner, 
+                                               LPSTR lpszPath, 
+                                               int nFolder, 
+                                                BOOL fCreate))
+                              GetProcAddress(gShell32DLLInst, 
"SHGetSpecialFolderPath");
+    }
+#endif
+}

You need a typedef. Even if you were using this type in only one place a typedef 
is prettier. But since you use it in two places, it is mandatory in my book. The 
param names are not required.

It would be good to add a comment about why this is needed; i.e. that 
GetSpecialPath is faster but not available without IE4 (or whatever). I'd 
also add a comment after the 'if' block at the end saying that it is falling 
back to do the same thing but by different means.

Comment 20

[Doug Turner (:dougt)]

there are no typedefs for similar constructs:

/xpcom/io/nsFileSpecOS2.cpp, line 819
/xpcom/io/nsLocalFileOS2.cpp, line 1694
/xpcom/io/nsFileSpecWin.cpp, line 690
/xpcom/io/nsLocalFileWin.cpp, line 1497

Comment 21

[Daniel Veditz [:dveditz]]

Personally I'd go with balleysson's suggestion and link with shfolder.dll
instead, and redistribute that .dll in the installer if it's not already present.

Then if you had to you could #if _MSC_VER < 1200 the old slow code to keep old
folks building. Other parts of the build already requires SP3 for VC6 people so
they should be OK.

Comment 22

[John Bandhauer]

dougt: five wrongs don't make a right. You *know* this is lame.

dveditz: I'm OK with linking to a new dll if it is freely redistributable and 
easily available. If we go that route then an announcement (with a link to the 
dll and how to install it) needs to be posted for developers. And, the 
installers need to be updated to do the deed before this change hits the tree 
again.

Comment 23

[Doug Turner (:dougt)]

jband: okay.  you got me.  I am a slacker.  I will fix it up.  In the meantime,
can you tell me if this work for you?

dveditz: You really want every mozilla windows distribution to have to ship this
library?

Comment 24

[Daniel Veditz [:dveditz]]

Sure, why not? It's only 20K (compresses to < 8K for distribution), and it's the
right way to do it according to MS. We could make it an optional install like
msvcrt.dll -- if you don't put it in the package the install won't fail, but if
you install on a platform without it then the browser will fail. Distributors
choice.  If people can build the damn thing (currently requires MSVC) then they
can redistribute shfolder.dll.

but I don't care much either way. If you prefer doing the GetProcAddress that's
fine, will save 8K on the package size at the cost of some speed for the lookup
and some code bloat for the backup method.

Comment 25

[Doug Turner (:dougt)]

I do not want to have to redistribute libraries.

Comment 26

[Bernard Alleysson]

You don't have to redistribute libraries (or it should be another bug).
Once this bug is fixed, every mozilla distribution can choose to distribute or
not to distribute shfolder.dll.

But, for this to be possible, please look up the symbol in shfolder.dll, not in
shell32.dll (this is *now* the right thing to do with windows 2000 and XP, and
ther is no need to redistribute anything).

If you want to target other OS like NT4 + IE4 (ie with newer version of
shell32.dll ) then you can look up in shell32.dll when the look up in
shfolder.dll has failed.

this should look like:

NS_COM void StartupSpecialSystemDirectory()
{
#if defined (XP_WIN)
    gShell32DLLInst = LoadLibrary("shfolder.dll");
    if(gShell32DLLInst)
    {
        getSpecialPathProc  = (BOOL (WINAPI *) (HWND hwndOwner, 
                                               LPSTR lpszPath, 
                                               int nFolder, 
                                                BOOL fCreate))
                              GetProcAddress(gShell32DLLInst,
"SHGetSpecialFolderPath");
    }
    
    if (!getSpecialPathProc)
    {
       if (gShell32DLLInst)
           FreeLibrary(gShell32DLLInst);

       gShell32DLLInst = LoadLibrary("shell32.dll");
       if(gShell32DLLInst)
       {
        getSpecialPathProc  = (BOOL (WINAPI *) (HWND hwndOwner, 
                                               LPSTR lpszPath, 
                                               int nFolder, 
                                                BOOL fCreate))
                              GetProcAddress(gShell32DLLInst,      
"SHGetSpecialFolderPath");
      }
   }
#endif
}

you could define the function (SHGetFolderPath) prototype with a typedef
and the code would be clearer

Comment 27

[John Bandhauer]

dougt: FWIW, I no longer have the machine on which this was a problem for me. 
So, I can't test for you. But others *do* still care about that platform. Like I 
said before... I don't care if there is a new dll requirement as long as the 
installers have a handle on the problem and people who build or run 
non-installer builds are warned and given clear instructions on where to get the 
new dll and how to install it.

Comment 28

[Doug Turner (:dougt)]

(I actually though that you were the last person to have NT without ie4. :-)

Bernard, I will incorporate your suggestion.

Comment 29

[Doug Turner (:dougt)]

Attachment: another round. v.3

Comment 30

[John Bandhauer]

Comment on attachment 55613 [details] [diff] [review]
another round.  v.3

sr=jband
Looks good to me.
I trust that the entry point really can be in one or
the other dll.
I'd probably do a silly loop thing for the two lookups to
avoid repeating code. But this is fine.

comment nit: 'writing' has only one 't'.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 55613 [details] [diff] [review]
another round.  v.3

r=dveditz

Comment 32

[Doug Turner (:dougt)]

Fix checked in:
Checking in nsSpecialSystemDirectory.cpp;
/cvsroot/mozilla/xpcom/io/nsSpecialSystemDirectory.cpp,v  <-- 
nsSpecialSystemDirectory.cpp
new revision: 1.52; previous revision: 1.51
done