FileManager re-factor

I've been looking at how FileManager is used in clang to figure out how best to use llvm::sys::PathWithStatus to replace large parts of FileEntry and it seems possibly all of DirectoryEntry.

I was hoping that I could get rid of the (per-file) directory stat by assuming that if the file existed, so does the directory but then I saw that we use the directory device and inode on unix-like systems as an index in UniqueDirContainer. Is the stat more expensive then hitting an llvm::StringMap to see if we've checked this exact directory yet? We seem to use the device/inode combo to account for sym-linked directories but we don't seem to try to resolve the symlinked directory to it's origin so as to avoid the FS stat on open. Is this something that should be implemented instead?

Also, the directory stat can be ifdef'd out for WIN32 as the WIN32 implementation of UniqueDirContainer doesn't make use of any of the stat elements.

I've already ben able to replace the path construction and parsing code in FileManager with calls to the appropriate Path member function which cleans things up nicely.

Thoughts?

John

I've been looking at how FileManager is used in clang to figure out
how best to use llvm::sys::PathWithStatus to replace large parts of
FileEntry and it seems possibly all of DirectoryEntry.

I was hoping that I could get rid of the (per-file) directory stat by
assuming that if the file existed, so does the directory but then I
saw that we use the directory device and inode on unix-like systems as
an index in UniqueDirContainer. Is the stat more expensive then
hitting an llvm::StringMap to see if we've checked this exact
directory yet?

I would imagine stat() is much more expensive.

We seem to use the device/inode combo to account for
sym-linked directories but we don't seem to try to resolve the
symlinked directory to it's origin so as to avoid the FS stat on open.
Is this something that should be implemented instead?

Sounds good. It would be useful to measure the difference to verify the performance improvement.

Also, the directory stat can be ifdef'd out for WIN32 as the WIN32
implementation of UniqueDirContainer doesn't make use of any of the
stat elements.

I've already ben able to replace the path construction and parsing
code in FileManager with calls to the appropriate Path member function
which cleans things up nicely.

Thoughts?

I don't believe FileManager has received much attention lately, so it's great to have you look into improving it.

I'm sure Chris will have some thoughts...

snaroff

I was hoping that I could get rid of the (per-file) directory stat by
assuming that if the file existed, so does the directory but then I
saw that we use the directory device and inode on unix-like systems as
an index in UniqueDirContainer. Is the stat more expensive then
hitting an llvm::StringMap to see if we've checked this exact
directory yet?

I would imagine stat() is much more expensive.

I think so as well, I'll circle back to this one after the others are done.

We seem to use the device/inode combo to account for
sym-linked directories but we don't seem to try to resolve the
symlinked directory to it's origin so as to avoid the FS stat on open.
Is this something that should be implemented instead?

Sounds good. It would be useful to measure the difference to verify the performance improvement.

I am currently using dtrace to track the number of stats performed by clang. I am sampling clang's 'make test' right now to get numbers but am wondering if there is a large-ish clang-friendly project that would be better suited for this task? Recommendations welcome.

dtrace command line:
  dtrace -n 'syscall::stat:entry /execname == "clang"/ { @[copyinstr(arg0)] = count() }' > trace-clang-stat.log

I will probably write a small bash script to start the above in the background, run make and then kill the above to write out the log. I'm currently unaware of a way to have dtrace execute a command to trace and then exit on its own but I'm new to dtrace so it probably just haven't fount it yet.

I don't believe FileManager has received much attention lately, so it's great to have you look into improving it.

No problem, it's my getting to learn clang project.

  - John

an index in UniqueDirContainer. Is the stat more expensive then
hitting an llvm::StringMap to see if we've checked this exact
directory yet?

I would imagine stat() is much more expensive.

I think so as well, I'll circle back to this one after the others are
done.

Also note that clang tries to just 'open' a file in certain cases and handle the error code instead of doing stat then doing the open on success.

We seem to use the device/inode combo to account for
sym-linked directories but we don't seem to try to resolve the
symlinked directory to it's origin so as to avoid the FS stat on
open.
Is this something that should be implemented instead?

Sounds good. It would be useful to measure the difference to verify
the performance improvement.

I am currently using dtrace to track the number of stats performed by
clang. I am sampling clang's 'make test' right now to get numbers but
am wondering if there is a large-ish clang-friendly project that would
be better suited for this task? Recommendations welcome.

dtrace command line:
  dtrace -n 'syscall::stat:entry /execname == "clang"/
{ @[copyinstr(arg0)] = count() }' > trace-clang-stat.log

This is very interesting. I tried out:

dtrace -n 'syscall:::entry /execname == "clang"/{ printf("%x %x", arg0, arg1); } syscall::stat:entry /execname == "clang"/{ printf("%s", (copyinstr(arg0))); } syscall::open_nocancel:entry /execname == "clang"/{ printf("%s", copyinstr(arg0)); }' | & less

to get a full trace. I see stuff that looks like this for Cocoa/Cocoa.h:

          stat:entry /Users/sabre/llvm/Debug/Headers/Cocoa
          stat:entry /usr/local/include/Cocoa
          stat:entry /usr/lib/gcc/i686-apple-darwin9/4.0.1/include/Cocoa
          stat:entry /usr/lib/gcc/powerpc-apple-darwin9/4.0.1/include/Cocoa
          stat:entry /usr/include/Cocoa
          stat:entry /System/Library/Frameworks/Cocoa.framework/Headers
          stat:entry /System/Library/Frameworks/Cocoa.framework/Headers/Cocoa.h
open_nocancel:entry /System/Library/Frameworks/Cocoa.framework/Headers/Cocoa.h

This pattern happens many times for each top level framework Within a framework, I see patterns like this:

          stat:entry /System/Library/Frameworks/Foundation.framework/Headers/NSArchiver.h
open_nocancel:entry /System/Library/Frameworks/Foundation.framework/Headers/NSArchiver.h
          stat:entry /System/Library/Frameworks/Foundation.framework/Headers/NSArray.h
open_nocancel:entry /System/Library/Frameworks/Foundation.framework/Headers/NSArray.h
          stat:entry /System/Library/Frameworks/Foundation.framework/Headers/NSEnumerator.h
open_nocancel:entry /System/Library/Frameworks/Foundation.framework/Headers/NSEnumerator.h

There are two things wrong with this:

1. We're doing a stat followed by an open. We should just do an open and on failure, continue the search. If the open succeeded and we need size info etc, clang should do an fstat on the file descriptor.

2. We end up "checking" some directories many times. For example, Cocoa.h checks /Users/sabre/llvm/Debug/Headers/ 82 times for files in my example, and only succeeds 5 times. It would be better to have DirectoryEntry get the file/dir contents of the directory after some number of queries using 'getdirentries'. This would save the repeated negative hits to a directory.

The first one is probably pretty easy, the second one is more involved. Both should only be done with careful attention to measured performance.

-Chris

Ah, here's another interesting thing we're doing wrong. Printing out the path to 'access' shows we also call 'access' unnecessarily for framework lookup (this is a lookup of CarbonCore/ConditionalMacros.h):

          stat:entry /Users/sabre/llvm/Debug/Headers/CarbonCore
          stat:entry /usr/local/include/CarbonCore
          stat:entry /usr/lib/gcc/i686-apple-darwin9/4.0.1/include/CarbonCore
          stat:entry /usr/lib/gcc/powerpc-apple-darwin9/4.0.1/include/CarbonCore
          stat:entry /usr/include/CarbonCore
          access:entry /System/Library/Frameworks/CarbonCore.framework/
          access:entry /Library/Frameworks/CarbonCore.framework/
          stat:entry /System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/
          stat:entry /System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers
          stat:entry /System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/ConditionalMacros.h
open_nocancel:entry /System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/ConditionalMacros.h

I don't see any reason to call 'access' on /S/L/F/CarbonCore.framework/ and /L/F/CarbonCore.framework if we're about to stat something underneath it.

Here's the dtrace command I used:
dtrace -n 'syscall:::entry /execname == "clang"/{ printf("%x %x", arg0, arg1); } syscall::stat:entry /execname == "clang"/{ print"%s", (copyinstr(arg0))); } syscall::open_nocancel:entry /execname == "clang"/{ printf("%s", copyinstr(arg0)); } syscall::access:entry /execname == "clang"/{ printf("%s", copyinstr(arg0)); }' | & tee out.log

-Chris

Interesting stuff... how did we live without these tools? :slight_smile:

2. We end up "checking" some directories many times. For example,
Cocoa.h checks /Users/sabre/llvm/Debug/Headers/ 82 times for files in
my example, and only succeeds 5 times. It would be better to have
DirectoryEntry get the file/dir contents of the directory after some
number of queries using 'getdirentries'. This would save the repeated
negative hits to a directory.

I'm a little paranoid about this, as this tradeoff could have a very
bad worst case, and it makes performance less predictable across
machines. However, for directories we expect to "control", like our
own headers directory, it would make sense to just provide a mechanism
to load the entire directory. Then we could investigate extending this
assumptions to directories we expect to have a controlled structure
(like frameworks).

Dynamically making this choice seems like asking for a bugzilla when
someone decides to drop their 6 headers into a directory with 10k
files.

- Daniel

getdirentries takes a "max number of bytes to read". If you call it once and it overflows a reasonable size, just consider the directory "too big to handle" and always stat future queries to it.

-Chris