100828 - nsLocalFileMac has serious problems

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

There are serious nsCOMPtr usage problems which cause preformance problem.  This
file needs a nice overhaul.

Comment 1

[Conrad Carlen (not reading bugmail)]

It needs an overhaul for other reasons as well - too many redundant instance
vars for one. I'm hoping to get to that in 0.9.6. What are the nsCOMPtr uses
which cause perf problems?

Comment 2

[Doug Turner (:dougt)]

great. over to you.  maybe a dup.

Check for cases where we assign our base class into a comptr for the fun of it.
 Also, for excessive QI's.

Comment 3

[Conrad Carlen (not reading bugmail)]

Reworking it to address these problems:

(1) It doesn't observe the followLinks attribute - perf problem because we
always attempt to check for and resolve aliases on every ResolveAndStat(). It
will have to default to TRUE for debug builds but could help on release builds.
(2) nsILocalFileMac does not have an .idl file and the iface does not inherit
from nsILocalFile. This is a huge pain. Whenever making an nsILocalFileMac with
an NS_NewLocalFileWithFSSpec, you have to QI the stupid thing before using it as
an nsIFile.
(3) The directory iterator is really inefficient - see this mess:
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsLocalFileMac.cpp#924
(4) Making better use of the cached cat info

This patch will also fix bug 28966

Comment 4

[Conrad Carlen (not reading bugmail)]

Attachment: prelim patch

Comment 5

[Conrad Carlen (not reading bugmail)]

Now it has an actual .idl file and descends from nsILocalFile which makes using
it much better - notice lack of annoying QI's in PPEmbed.

Two things I'm not sure about:
(1) fileCreator and type are separate attributes which seems right to me.
Although, most consumers get both at the same time so it adds a line of code
here and there.
(2) There is a stack object which sets and restores the state of the followLinks
attribute. This saves a few lines of code an restores the state on early
function exit. It's defined locally in nsLocalFileMac.cpp. Should it be in the
header of nsILocalFile?

Comment 6

[Conrad Carlen (not reading bugmail)]

I took some before patch/after patch performance measurements. I measured the
time it took for xptiInterfaceInfoManager::BuildFileList() to enumerate the
components dir which happens twice each time at startup. I did this with a debug
build but I resolved all the aliases using drop stuff/stuffit expander to
simulate a release build. In both cases, the 1st scan of the dir takes more time
than the 2nd (probably because of OS level file mgr caching)

                  1st scan      2nd scan
before patch:
                  101 ticks     24 ticks
after patch:      3 ticks       2 ticks

That means this patch saves 120 ticks (2 seconds!) on directory enumeration on
every freakin' launch. And this was only the enumeration done by
xptiInterfaceInfoManager::BuildFileList() Caveat: in a debug build, there are
more items in the components dir because xSYM files are present but still, the
gain is pretty big.

I'm posting a new patch to nsLocalFileMac as some bugs were discovered in the
prelim patch.

Comment 7

[Conrad Carlen (not reading bugmail)]

Attachment: updated patch to nsLocalFileMac

Comment 8

[Simon Fraser [no longer active]]

Conrad: there are problems with the cached CInfoPBRec. Basically, it's impossible 
for us to maintain the dirty state of this cache, because calls outside of 
nsLocalFileMac can invalidate it. For example, we have code that uses the 
OpenNSPRFileDesc (or whatever) to open a file for writing outside of this code. 
Writing to the file will invalidte the cached CInfoPBRec, but there's no way we 
can know that it's changed (other than to check the mod date, which you'd do with 
PBGetCatInfo anyway).

Note also that there are some new file APIs using FSRefs which allow for getting 
file info in batches. This would be good for the enumerators.

Comment 9

[Suresh Duddi (gone)]

Also xpti enumration happens only for debug builds (right jband ?) There are
other enumerations happening though on release builds. Will this patch save on
any enumeration ?

Comment 10

[John Bandhauer]

dp: yes, the xpt file enumeration happens only in debug builds, if the xpti.dat 
is unreadable, or we are doing an explicit xpcom autoregistration. If it is a 
useful benchmark, then great. But don't count on speeding up this particular 
directory enumeration to matter for release builds.

Comment 11

[Conrad Carlen (not reading bugmail)]

> Conrad: there are problems with the cached CInfoPBRec.

* I could probably ditch that optimization. I tested the performance again,
forcing UpdateCachedCatInfo() to always update and it did not lessen the perf
gain of enumeration. The current enumeration code is slow for several other reasons.
* Still, we could keep it and be smarter about when to invalidate it. The
methods IsFile() and IsDirectory() are called a lot. Even if something opens the
file and changes it, whether it's a file or directory is not going to change.
So, GetFileSize() and GetLastModificationDate() could always force an update of
the cached info, whereas IsFile() and IsDirectory() would not.


> Note also that there are some new file APIs using FSRefs

Those will be used when the nsLocalFile impl is factored into Classic, HFSPlus,
and OSX versions. There's already too much piecemeal OS X code in here now.

> There are other enumerations happening though on release builds. Will this
> patch save on any enumeration ?

Yes. I measured xptiInterfaceInfoManager::BuildFileList() because it does little
else other than the enumeration so measuring it is close to measuring only the
enumeration. But still, any enumeration done at startup will benefit.

Comment 12

[Conrad Carlen (not reading bugmail)]

Attachment: updated patch to mozilla/xpcom/io

Comment 13

[Conrad Carlen (not reading bugmail)]

The update contains improvements and fixes after running it through
http://jimbob/trigger3.html. The xpinstall test script is good because it makes
extensive use of nsILocalFile.

Another change: UpdateCachedCatInfo takes a boolean parameter with which an
update can be forced. This is used in most places - particularly when getting
the date, size, or existence. The original code was caching this info far too
long. See mHaveFileInfo. The only thing which reset this member was MakeDirty()
which reset everything which isn't nescesary. Even with updating the cached cat
info more frequently, dir enumeration is still tremendously faster. 

Comment 14

[Simon Fraser [no longer active]]

Conrad: can you obsolete the old patches please?

Comment 15

[Conrad Carlen (not reading bugmail)]

Simon, the prelim patch contains tree-wide changes for nsILocalFileMac API
changes. At this point, those will not change. The changes in attachment 54483 [details]
supercede any changes in the first patch to mozilla/xpcom/io. It's not quite
ready for review yet, though. I'm currently getting rid of the internal use of
eInitWithPath/eInitWithFSSpec. Assuming that works out, there'll be another patch.

Comment 16

[Conrad Carlen (not reading bugmail)]

*** Bug 104248 has been marked as a duplicate of this bug. ***

Comment 17

[Conrad Carlen (not reading bugmail)]

I have a new patch coming up which does a few things:

(1) Gets rid of eInitializedWithPath, eInitializedWithFSSpec distinction. When
initialized with a full patch, the existant portion of the path is resolved into
a spec and the rest, which doesn't exist, is kept in mAppendedPath. When
initializing with a full path, at least the volume must exist or else it's an error.
(2) The object used to contain 3 FSSpecs and 3 path member vars - not very
concise. The object now contains 2 specs and only one path. The reason for 2
specs is so that, is the leaf is an alias, we can toggle back and forth between
resolving the alias or not.
(3) When Append() is called, the current leaf is resolved if it is an alias.
Before this wasn't done and alises were only resolved at the leaf when the patch
was finally built up. If one of the appended nodes was an alias, it would fail.
The problem, to a user, was that you could not make an alias of your cache
directory (or much of anything else) Now you can.

Comment 18

[Conrad Carlen (not reading bugmail)]

Attachment: new tree-wide patch

Comment 19

[Conrad Carlen (not reading bugmail)]

This one is ready for review. Another thing I would like to do, since so much of
this is being rewritten, is to get rid of the tabs that have always been present
thoughout this file. As you can see, it makes diffs ugly if the new code uses
spaces. I could do one of two things - reviewers take your choice:
1. review as-is and, before checking in, I'll detab it with BBEdit.
2. detab it now and post a new diff made with -w

Comment 20

[Simon Fraser [no longer active]]

Do this one:
1. review as-is and, before checking in, I'll detab it with BBEdit.

Comment 21

[Conrad Carlen (not reading bugmail)]

Alright, then review away. I'll detab it before checking in.

Comment 22

[Conrad Carlen (not reading bugmail)]

*** Bug 28966 has been marked as a duplicate of this bug. ***

Comment 23

[Simon Fraser [no longer active]]

Comments:
+    attribute OSType fileType;
+    attribute OSType creator;

Use 'fileCreator' for consistency?

+    char        mAutoBuffer[512];
+    char        *mAllocatedBuffer;
+    char        *mNewString;

We really need a class that does this stack/buffer thing transparently. It's used 
in so many places.

+        return nsCRT::strtok(mAllocatedBuffer ? mAllocatedBuffer : mAutoBuffer, 
":", &mNewString);

Why not store a pointer than can point to either the allocated buffer, or the 
stack buffer, and avoid this if/else test all the time? You'd just have to set it 
up in the ctor.

The rest looks OK, but this patch needs some thorough testing, since it's so 
core. I'd even suggest testing an installer, to make sure that installation 
didn't break.

sr=sfraser

Comment 24

[Conrad Carlen (not reading bugmail)]

> Use 'fileCreator' for consistency?

OK

> Why not store a pointer than can point to either the allocated buffer

OK - It was to save an instance var in favor of the low expense of the if/else.
Now, I think what you suggest is better.

As far as testing with the installer, I did test successfully with
http://jimbob/trigger3.html which hits every obscure case.

I still need an r= on this. Patrick or Doug? Since the changes mentioned above
are slight, please review the current patch.

Comment 25

[Conrad Carlen (not reading bugmail)]

CC'ing rjc for review. This has had sr= for a while. Now if I can get an r=.

Comment 26

[Robert John Churchill]

o  attribute OSType creator;

I second the "fileCreator" instead of "creator" change.  I assume that the
method name (not just the variable name) was also changed ... GetFileCreator()
instead of GetCreator()

o  *outIsWritable = !(mCachedCatInfo.hFileInfo.ioFlAttrib & 0x01);

Nit:  instead of 0x01, suggest using the correct mask from <Files.h>
      which looks to be kioFlAttribLockedMask


Also, I've always been quite annoyed by our alias resolution handling on Mac. 
SO... I'd like to see the following test done.  :)

1)  mount a remote Appletalk volume (call it _VOL_ for short)
2)  make an alias to that remote volume (call it _AL_ for short)
3)  unmount the remote volume
4)  place the alias in some directory (call it _DIR_ for short) on the local
hard disk
5)  with Mozilla, type in the appropritate file:/// URL to view the contents of
directory _DIR_

If Mozilla seems to go to sleep for up to two minutes (as it incorrectly tries
to mount _VOL_) or anything action other than merely listing the alias _AL_
itself, then something is still wrong with alias resolution.

With the above changes, and IFF Mozilla passes the above test correctly, r=rjc

Comment 27

[Conrad Carlen (not reading bugmail)]

I'll do the first two points.

As far as this:
1)  mount a remote Appletalk volume (call it _VOL_ for short)
2)  make an alias to that remote volume (call it _AL_ for short)
3)  unmount the remote volume
4)  place the alias in some directory (call it _DIR_ for short) on the local
hard disk
5)  with Mozilla, type in the appropritate file:/// URL to view the contents of
directory _DIR_

 It currently defaults to resolving aliases. It has to for most consumers to
work. One thing this patch does is to make nsLocalFile obey the followLinks
attribute. With this patch, you could at least set the follow links attribute to
FALSE on each entry you got back in the enumeration and it would actually do
something.
 I think what would be more convenient and offer better control would be to add
a param to GetEntries() specifying whether or not to resolve aliases on each
entry that's returned. You  may want to resolve the alias to the dir being
enumerated but not resolve the children. That's another bug though.

 

Comment 28

[Conrad Carlen (not reading bugmail)]

Robert, I will try putting a call to SetFollowLinks(PR_FALSE) in the directory
listing generator as soon as it gets back an entry and make sure it does the
right thing.

Comment 29

[Robert John Churchill]

Conrad:  SetFollowLinks(PR_FALSE) is already in the RDF file system datasource
which is what actually ends up handling file:/// URL display.  :)  So the test
should be a good one to do.

Comment 30

[Conrad Carlen (not reading bugmail)]

At this point, right:
http://lxr.mozilla.org/mozilla/source/rdf/datasource/src/nsFileSystemDataSource.cpp#1215
That's not quite going to do it. It should be done also on line 1233. Say in
this case, aDirLocal was an alias to a directory. You may want to resolve that.
But, what you don't want resolved are its entries. The additional call on line
1233 will do that.

Comment 31

[Robert John Churchill]

Agreed on line # 1233 due to the IsHidden() call; however I don't know that I
agree that aliases to directories should ever be resolved. Sticky issue... if I
wanted to delete the alias to a directory (assuming that we actually had that
functionality, that is), I wouldn't want the real directory to be blown away. 
Anyway, different issue for a different time/bug.  :)

Comment 32

[Conrad Carlen (not reading bugmail)]

Mass move to 0.9.7

Comment 33

[Conrad Carlen (not reading bugmail)]

Better to get something like this into the beginning of 0.9.8 than end of 0.9.7

Comment 34

[Conrad Carlen (not reading bugmail)]

Attachment: updated patch

Update to the last patch - no significant changes, just updated for changes in
the tree. nsLocalFileMac.cpp was de-tabbed as this is the final which will be
checked in.

Comment 35

[Conrad Carlen (not reading bugmail)]

Checked in. nsLocalFileMac was de-tabbed so pretty much everything is different.
After the tree closed, I remembered that the old nsILocalFileMac.h needs to be
removed. I'll leave this open until I do that later today.

Comment 36

[Conrad Carlen (not reading bugmail)]

The handmade nsILocalFileMac.h is removed so marking fixed.