73214 - filesystem datasource spends half of its time in FileSystemDataSource::isDirURI()

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect, P3)

Description

[Chris Waterson]

I was a bit disappointed in the performance of the RDF outliner, so I profiled 
it on a hacked version of directory.xul, which I'll attach to the bug. (Replace 
xpfe/components/directory/directory.xul with that version, and rebuild in your 
xpfe/components dir.)

It turns out the *half* the time spent scrolling around is spent in 
FileSystemDataSource::isDirURI(). Looking at isDirURI(),

  - 1/3 of the time is spent in NS_NewURI(), constructing a file: URL

  - 1/3 of the time is spent in nsLocalFile::IsDirectory(), which is
    all ends up in PR_GetFileInfo64().

  - 1/6 of the time is spent in nsStdURL::GetFile().

  - 1/6 of the time is spent destroying all these wonderful XPCOM objects
    that we've created to test if something is a directory.

I remember rjc complaining about how bad performance got when he converted the 
filesystem datasource to nsIFile from nsFileSpec. dougt always said it was rjc's 
fault, and mumbled about how rjc was doing stuff wrong. Regardless, this sucks. 
kandrot: you are nsIFile whipping boy these days. Make it better!

Comment 1

[Chris Waterson]

Attachment: hacked directory viewer that uses RDF outliner

Comment 2

[Doug Turner (:dougt)]

The GetFileInfo64 is going to be required for the stat.  I believe that this
does more work that just a vanilla stat().  I guess we could optimize it so that
only the required information is pulled out for the isExists, isDir, isFile,
ect.  

Conrad and I have been sitting on a patch for some times now that should help
out this problem.  kandrot, please updating it and check it in. (56354) 
r=dougt.  Waterson, please sr so that we can get this in asap.

I will attached a patch that will shave a 1/3 of the time off but is dependant
on implementing SetURL on the nsIFile (see above). 

Comment 3

[Doug Turner (:dougt)]

Attachment: Patch to use NSIFile::LSetURL instead of NS_NEWURI

Comment 4

[Chris Waterson]

Nice. I suspect we could even improve on this a bit by just making an 
nsILocalFile member variable that we use over and over for the purpose of 
converting paths. (It's not like this code is threadsafe.) Adding dependency to 
bug 56354.

Comment 5

[Edward Kandrot]

Well, since this patch calls a function that is not yet implemented, I can not
see checking it in yet.  The bug this depends on is marked 1.0, so I am updating
this to be the same.

Comment 6

[Doug Turner (:dougt)]

kandrot, there is a patch in the other bug.  Lets get this fixed!  If you can't,
please reassign to me.

Comment 7

[Edward Kandrot]

Yes, there is a patch in this bug, but it calls a function that is not
implemented.  Based on this, I would assume that it would not work.  It is all
yours to check in, if you believe that it works.

Comment 8

[Doug Turner (:dougt)]

.

Comment 9

[Gagan]

->jtaylor

Comment 10

[Doug Turner (:dougt)]

John, I think that adamlock is going to make this really easy for you.  He has a 
patch in 53654 which will allow you to directly call SetURL/GetURL on a nsIFile.

Comment 11

[Adam Lock]

My patch is just for GetURL. Conrad implemented SetURL in his more substantial 
patch attached to bug 53654.

Comment 12

[John Taylor]

Attachment: New isDirURI patch

Comment 13

[John Taylor]

Attachment: New isDirURI patch

Comment 14

[John Taylor]

Sorry about the double attachments. Doug, can you review the patch?

Comment 15

[Doug Turner (:dougt)]

assuming that this has been tested xp, sr=dougt.

Comment 16

[John Taylor]

retargeting. 

Comment 17

[John Taylor]

Fix checked in.

Comment 18

[benc]

tever is not RDF QA anymore