456662 - optimize Mac OS X filesystem code and prepare for 64-bit

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Josh Aas]

We should make the following improvements to our Mac OS X filesystem code:
- optimize for speed
- minimize deprecated API usage and move towards an implementation that will work under 64-bit Mac OS X
- remove dependencies on MoreFiles and FSCopyObject

Comment 1

[Josh Aas]

Attachment: fix v0.8

Comment 2

[Josh Aas]

Attachment: fix v0.8.5

Comment 3

[Josh Aas]

Attachment: file removal v1.0

To slim down the bigger patch, here are a bunch of file removals we can just do now.

Comment 4

[Josh Aas]

Attachment: file removal v1.1

more dead code

Comment 5

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 340333 [details] [diff] [review]
file removal v1.1

sr=pink

Comment 6

[Josh Aas]

"file removal v1.1" landed on trunk

Comment 7

[Josh Aas]

Attachment: fix v0.9

wip backup

Comment 8

[Josh Aas]

Attachment: fix v0.9.1

Comment 9

[Josh Aas]

Attachment: fix v1.0

This is most of the work we need to do on our Mac OS X filesystem code for now. There is still a little more to be done (we could replace more FSRef code, implement some more unimplemented methods, do API cleanup), but I'd rather do more work in separate patches.

This patch drops Ts by ~1.4%, consistent with what I thought might be possible from my original profiling. All of the nsLocalFile functions that showed up near the top of my initial profile have been knocked way down.

This work also replaces a lot of old code, including the whole MoreFiles system, much of which uses deprecated APIs and is not 64-bit portable.

16 files changed, 760 insertions(+), 8212 deletions(-)

I also implemented some previously unimplemented methods related to symlinks, and fixed a leak-on-error case.

The only major behavioral change I made was to symlink/alias handling. Basically with this patch our Mac OS X nsLocalFile impl behaves much more like the UNIX impl with respect to symlinks. They are always followed, except where the API specifies that they are not, and old Mac OS aliases are basically disregarded. I looked at users that might rely on our old behavior and didn't see any. I think this is the right way to go.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hey, while you're here, is there any way to make the nsLocalFile::Normalize method *not* require that the file actually exists? We need to figure out a way to avoid realpath, or at least handle its failure gracefully.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 340834 [details] [diff] [review]
fix v1.0

I didn't test this patch, or even try to compile it.

Rather, I just eyeballed it for correctness.  I didn't find any
problems.

Whenever the patch simply moved original code, I checked to make sure
the moved code hadn't been changed accidentally.  And where code was
changed (for example to use Cocoa methods), I usually just looked at
the new methods' names to make sure they seemed appropriate, rather
than always checking those new methods' documentation.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 340834 [details] [diff] [review]
fix v1.0

it is really hard to review this.

Lets do this:

Upload the nsLocalFile implementation (not patches).  after all, it is a rewrite effectively.

seperate out the hg file removes and build config junk into a different patch.  That will make it easier on all of us to review the substance of this changeset.

Comment 13

[Josh Aas]

Attachment: fix v1.0 nsLocalFileOSX.h

Comment 14

[Josh Aas]

Attachment: fix v1.0 nsLocalFileOSX.mm

Comment 15

[Josh Aas]

Attachment: fix v1.0 other

Comment 16

[Doug Turner (:dougt)]

steven could you also review these changes (it is probably easier now for review)

Comment 17

[Doug Turner (:dougt)]

Some of these comments may not be your changes because I am review the whole file, but since your are touching most of this file, i figured you should fix up mistakes:

OOC, couldn't you mostly use the unix implementation?

spacing is off on the first line of this macro; 

#define CheckPath()                         \
    PR_BEGIN_MACRO                           \
        if (!mURL)                           \
            return NS_ERROR_NOT_INITIALIZED; \
    PR_END_MACRO



in nsDirEnumerator::Init()

you will leak mFSRefsArray if you return early due to a failure.

Why are you using nsMemory::Alloc instead of just malloc?


nsLocalFile::AppendNative should not fail if mURL is null.  it shoudl be okay to construct a new nsILocalFile, then append (/foopy/x) and have that work.

GetFileSizeOfLink()

do links actually have varying file sizes?

SetPermissionsOfLink() 

Do you just want to mark this as NOT IMPL similar to unix?



Much of the mac code should be review buy a stronger mac hacker than I.

With NS_NewLocalFile, NS_NewNativeLocalFile, and NS_NewLocalFileWithFSRef.  Please test that *result isn't null.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #16)

I'll do a new review when Josh is done with your changes.

Comment 19

[Josh Aas]

(In reply to comment #17)
> OOC, couldn't you mostly use the unix implementation?

We could use some of it, but there is still a lot of stuff in our Mac OS X impl that is different. If we tried to use the unix impl we'd really make a mess of it with ifdefs. I think separate impls is still the right thing to do for now, though we may be able to merge in the future.

> spacing is off on the first line of this macro; 

Fix coming up.

> you will leak mFSRefsArray if you return early due to a failure.

Fix coming up. (I didn't do this)

> Why are you using nsMemory::Alloc instead of just malloc?

Fix coming up. (I didn't do this)
> 
> nsLocalFile::AppendNative should not fail if mURL is null.  it shoudl be okay
> to construct a new nsILocalFile, then append (/foopy/x) and have that work.

Fix coming up.

> do links actually have varying file sizes?

I don't know, but even if it only has one size we might as well give the real size so long as we have an API that is supposed to do it and we're capable.

> SetPermissionsOfLink() 
> 
> Do you just want to mark this as NOT IMPL similar to unix?

Mac OS X allows us to set permissions on the link and our API is supposed to do it if possible, so we might as well.

> Much of the mac code should be review buy a stronger mac hacker than I.

Steven will look over this again.

> With NS_NewLocalFile, NS_NewNativeLocalFile, and NS_NewLocalFileWithFSRef. 
> Please test that *result isn't null.

Fix coming up.  (I didn't do this)

Comment 20

[Josh Aas]

Attachment: fix v1.1

Comment 21

[Josh Aas]

Attachment: fix v1.1 nsLocalFileOSX.mm

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 341519 [details] [diff] [review]
fix v1.1

Once again I didn't test this patch, or try to compile it.

And where it just moves code (or reformats it), I just checked that
this didn't result in unexpected or incorrect changes.

But this time I did check the docs for every newly added OS call
(Carbon, Cocoa, or otherwise).

I found one very minor error:

  +  PRInt32 aliasSize = GetAliasSizeFromPtr(&aliasHeader);

  should be

  +  PRInt32 aliasSize = ::GetAliasSizeFromPtr(&aliasHeader);

I also have a couple questions:

1) nsLocalFileOSX supports (and did support) a SetFileSize() call.

   This seems inherently very dangerous.  How exactly will the OS
   calls (used on different platforms) accomplish this?  And have we
   eliminated the possibility of file-system corruption?

   The current code does (and did) use ::FSSetForkSize(), which seems
   safe enough (since there's a specific OS call whose only use is to
   reset the "fork size").  But (oddly) ::FSOpenFork() doesn't specify
   a "forkName" (it sets this parameter to nsnull).  And Apple's docs
   on this API (in the Cocoa File Manager Reference) don't say what
   happens when you do this.

2) IsExecutable() does (and did) use a Launch Services call
   (::LSCopyItemInfoForRef() to determine whether or not a file is
   executable.

   But do we know that Launch Services will consider a BSD executable
   (e.g. 'ls') to be executable?  And does it matter?

Comment 23

[Steven Michaud [:smichaud] (Retired)]

>   And Apple's docs
>   on this API (in the Cocoa File Manager Reference) don't say what
>   happens when you do this.

in the *Carbon* File Manager Reference

Comment 24

[Josh Aas]

(In reply to comment #22)
> I found one very minor error:
> 
>   +  PRInt32 aliasSize = GetAliasSizeFromPtr(&aliasHeader);
> 
>   should be
> 
>   +  PRInt32 aliasSize = ::GetAliasSizeFromPtr(&aliasHeader);

I'll fix on checkin or the next patch update.

> 1) nsLocalFileOSX supports (and did support) a SetFileSize() call.
> 
>    This seems inherently very dangerous.  How exactly will the OS
>    calls (used on different platforms) accomplish this?  And have we
>    eliminated the possibility of file-system corruption?
> 
>    The current code does (and did) use ::FSSetForkSize(), which seems
>    safe enough (since there's a specific OS call whose only use is to
>    reset the "fork size").  But (oddly) ::FSOpenFork() doesn't specify
>    a "forkName" (it sets this parameter to nsnull).  And Apple's docs
>    on this API (in the Cocoa File Manager Reference) don't say what
>    happens when you do this.

We should look into this, but I didn't touch this in my patch and I don't want to make any more changes until after it lands.

> 2) IsExecutable() does (and did) use a Launch Services call
>    (::LSCopyItemInfoForRef() to determine whether or not a file is
>    executable.
> 
>    But do we know that Launch Services will consider a BSD executable
>    (e.g. 'ls') to be executable?  And does it matter?

I believe we have a bug on file for this. I didn't change this code in my patch, I don't want to deal with that until after this lands.

Comment 25

[Josh Aas]

Just noting... Another reason this patch is good for perf is that doing FSRef->path or path->FSRef hits the disk. An FSRef is a reference number for a file on disk, it does not contain a path, so if you want to go from one to the other you have to make a system call and hit the disk.

Many of the methods in our impl are doing this:

nsILocalFile::Foo(path) {
  myFSRef = path->FSRef // hit the disk
  MacOSFoo(myFSRef)     // hit the disk again
}

which means that we make a system call and hit the disk (well, minus any kernel caching) before we get to doing any actual operations on a file. This patch gets rid of most of that.

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 341519 [details] [diff] [review]
fix v1.1

make sure perf does not dip.  especially look at ts.

you also ran mochitests, right?

nit: in a couple places you are still doing:

if (something != 0)

instead of

if (!something)



not your change, but is 1024 too big?

  // On 10.2, huge paths also crash CFURLGetFSRef()
  if (fixedPath.Length() > MAX_PATH_SIZE)

Comment 27

[Josh Aas]

(In reply to comment #26)
> (From update of attachment 341519 [details] [diff] [review])
> make sure perf does not dip.  especially look at ts.

In my local and tryserver tests Ts went down, but I'll be paying attention on checkin of course :)

> you also ran mochitests, right?

Yes, they were fine.

> not your change, but is 1024 too big?
> 
>   // On 10.2, huge paths also crash CFURLGetFSRef()
>   if (fixedPath.Length() > MAX_PATH_SIZE)

According to the research I did 1024 is the right choice. There were conflicting opinions though.

Comment 28

[Josh Aas]

landed on trunk

Comment 29

[Josh Aas]

backed out due to unit test failures

Comment 30

[Doug Turner (:dougt)]

please paste the diff that fixes the unit tests.

Comment 31

[Josh Aas]

Attachment: fix v1.2

pick up another build fix that we need, this doesn't fix the unit tests

Comment 32

[Josh Aas]

Attachment: fix v1.3

Fixes a problem with CopyInternal that caused a bunch of unit test failures. Still some unit tests failing.

Comment 33

[Josh Aas]

I've decided to break this patch up and land it in pieces over the next few months.

Comment 34

[Josh Aas]

With bug 489015 landing I think we can close this out. The Mac OS X filesystem code is 64-bit ready and a lot of the performance work originally done here has been landed.