Closed Bug 456662 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(2 files, 12 obsolete files)

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
Summary: update Mac OS X filesystem code → optimize Mac OS X filesystem code and prepare for 64-bit
Attached patch fix v0.8 (obsolete) — Splinter Review
Attached patch fix v0.8.5 (obsolete) — Splinter Review
Attachment #340044 - Attachment is obsolete: true
Attached patch file removal v1.0 (obsolete) — Splinter Review
To slim down the bigger patch, here are a bunch of file removals we can just do now.
Attachment #340186 - Flags: superreview?(roc)
more dead code
Attachment #340186 - Attachment is obsolete: true
Attachment #340186 - Flags: superreview?(roc)
Attachment #340333 - Flags: superreview?(roc)
Comment on attachment 340333 [details] [diff] [review]
file removal v1.1

sr=pink
Attachment #340333 - Flags: superreview?(roc) → superreview+
"file removal v1.1" landed on trunk
Attached patch fix v0.9 (obsolete) — Splinter Review
wip backup
Attachment #340079 - Attachment is obsolete: true
Attached patch fix v0.9.1 (obsolete) — Splinter Review
Attachment #340768 - Attachment is obsolete: true
Attached patch fix v1.0 (obsolete) — Splinter Review
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.
Attachment #340776 - Attachment is obsolete: true
Attachment #340834 - Flags: review?(smichaud)
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 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.
Attachment #340834 - Flags: review?(smichaud) → review+
Attachment #340834 - Flags: superreview?(doug.turner)
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.
Attachment #340834 - Flags: superreview?(doug.turner) → superreview-
Attached file fix v1.0 nsLocalFileOSX.h (obsolete) —
Attached file fix v1.0 nsLocalFileOSX.mm (obsolete) —
Attached patch fix v1.0 other (obsolete) — Splinter Review
Attachment #341070 - Flags: superreview?(doug.turner)
steven could you also review these changes (it is probably easier now for review)
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.
Attachment #341070 - Flags: superreview?(doug.turner) → superreview-
(In reply to comment #16)

I'll do a new review when Josh is done with your changes.
(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)
Attached patch fix v1.1 (obsolete) — Splinter Review
Attachment #340834 - Attachment is obsolete: true
Attachment #341519 - Flags: review?(smichaud)
Attached file fix v1.1 nsLocalFileOSX.mm (obsolete) —
Attachment #341070 - Attachment is obsolete: true
Blocks: 438694
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?
Attachment #341519 - Flags: review?(smichaud) → review+
>   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
(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.
Attachment #341519 - Flags: superreview?(doug.turner)
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 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)
Attachment #341519 - Flags: superreview?(doug.turner) → superreview+
(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.
landed on trunk
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 462036
backed out due to unit test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
please paste the diff that fixes the unit tests.
Attached patch fix v1.2 (obsolete) — Splinter Review
pick up another build fix that we need, this doesn't fix the unit tests
Attachment #341068 - Attachment is obsolete: true
Attachment #341071 - Attachment is obsolete: true
Attachment #341519 - Attachment is obsolete: true
Attachment #341520 - Attachment is obsolete: true
Attached patch fix v1.3Splinter Review
Fixes a problem with CopyInternal that caused a bunch of unit test failures. Still some unit tests failing.
Attachment #345200 - Attachment is obsolete: true
I've decided to break this patch up and land it in pieces over the next few months.
Blocks: 468509
Depends on: 464362
Depends on: 476230
Depends on: 489015
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.