Closed
Bug 786141
Opened 12 years ago
Closed 9 years ago
Use nsIFile.exists() instead of stat to check the existence of the file
Categories
(MailNews Core :: Database, defect)
MailNews Core
Database
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 40.0
People
(Reporter: hiro, Assigned: aceman)
Details
Attachments
(1 file, 3 obsolete files)
2.00 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
I am not sure why the stat is there.
Attachment #655853 -
Flags: review?(mbanner)
Comment 1•12 years ago
|
||
Comment on attachment 655853 [details] [diff] [review] Fix Review of attachment 655853 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay in looking at this. I think the general idea is fine, we just need some tweaks. ::: mailnews/db/msgdb/src/nsMsgDatabase.cpp @@ +1240,5 @@ > if (m_mdbEnv) > m_mdbEnv->SetAutoClear(true); > + dbFile->GetNativePath(m_dbName); > + bool exists = false; > + dbFile->Exists(&exists); There's a known bug on windows that Exists can cache the status of the file producing an out of date result. So I think we either need to clone the nsIFile (which we've done in other places) or keep passing the string and create a new nsIFile for this Exists test.
Attachment #655853 -
Flags: review?(mbanner)
Attachment #655853 -
Flags: review-
Attachment #655853 -
Flags: feedback+
Comment 2•10 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #1) > ::: mailnews/db/msgdb/src/nsMsgDatabase.cpp > @@ +1240,5 @@ > > if (m_mdbEnv) > > m_mdbEnv->SetAutoClear(true); > > + dbFile->GetNativePath(m_dbName); > > + bool exists = false; > > + dbFile->Exists(&exists); > > There's a known bug on windows that Exists can cache the status of the file > producing an out of date result. > > So I think we either need to clone the nsIFile (which we've done in other > places) or keep passing the string and create a new nsIFile for this Exists > test.
Assignee: nobody → hiikezoe
Flags: needinfo?(hiikezoe)
Reporter | ||
Comment 3•10 years ago
|
||
Create a new nsIFile instance in OpenMDB to check existence of the nsIFile as mark suggested in comment 1.
Attachment #655853 -
Attachment is obsolete: true
Attachment #8374509 -
Flags: review?(mbanner)
Flags: needinfo?(hiikezoe)
Reporter | ||
Comment 4•10 years ago
|
||
Try server results: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b9ff9288a76b There are lots of oranges but not related to this patch, I think.
Comment 5•10 years ago
|
||
Comment on attachment 8374509 [details] [diff] [review] Revised patch Review of attachment 8374509 [details] [diff] [review]: ----------------------------------------------------------------- Heading along the right lines, but I'd like a couple of additional checks added in please. ::: mailnews/db/msgdb/src/nsMsgDatabase.cpp @@ +1336,5 @@ > > if (m_mdbEnv) > m_mdbEnv->SetAutoClear(true); > m_dbName = dbName; > + nsCOMPtr<nsIFile> dbFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID); Should be checking that this gets created. @@ +1337,5 @@ > if (m_mdbEnv) > m_mdbEnv->SetAutoClear(true); > m_dbName = dbName; > + nsCOMPtr<nsIFile> dbFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID); > + dbFile->InitWithNativePath(nsDependentCString(dbName)); dbName should probably get a null-check as otherwise this could crash.
Attachment #8374509 -
Flags: review?(mbanner)
Attachment #8374509 -
Flags: review-
Attachment #8374509 -
Flags: feedback+
Comment 6•10 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #5) > Comment on attachment 8374509 [details] [diff] [review] > Revised patch > > Review of attachment 8374509 [details] [diff] [review]: > ----------------------------------------------------------------- > > Heading along the right lines, but I'd like a couple of additional checks > added in please. > ... Is this also a minor perf improvement? (consider, some users have many hundreds of folder)
Status: NEW → ASSIGNED
Flags: needinfo?(hiikezoe)
Comment 7•9 years ago
|
||
kent, it seems to me like this is an important patch but hasn't gotten the recognition. Am I wrong? And is there a better choice than you to finish it off?
Status: ASSIGNED → NEW
Flags: needinfo?(hiikezoe) → needinfo?(rkent)
Comment 8•9 years ago
|
||
I'm not much of a file system expert, but it is not clear to me that stat is a real improvement over Exists. Maybe Chiaki has some thoughts?
Flags: needinfo?(rkent) → needinfo?(ishikawa)
(In reply to Kent James (:rkent) from comment #8) > I'm not much of a file system expert, but it is not clear to me that stat is > a real improvement over Exists. Maybe Chiaki has some thoughts? The patch is the other way round. Removing stat in favour of nsIFile.exists(). Which is probably more preferred option. This is the single occurence of stat() in c-c. m-c has more of them so I don't know. Neil, what do you think?
Flags: needinfo?(neil)
Comment 10•9 years ago
|
||
(In reply to aceman from comment #9) > (In reply to Kent James from comment #8) > > I'm not much of a file system expert, but it is not clear to me that stat is > > a real improvement over Exists. Maybe Chiaki has some thoughts? > > The patch is the other way round. Removing stat in favour of > nsIFile.exists(). Which is probably more preferred option. This is the > single occurence of stat() in c-c. m-c has more of them so I don't know. > > Neil, what do you think? Exists() should be an improvement since it calls access rather than a full stat. (Even on Windows it should be better because it calls GetFileAttributesW rather than a full stat (stat is emulated by the CRT on Windows).)
Flags: needinfo?(neil)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #7) > kent, it seems to me like this is an important patch but hasn't gotten the > recognition. Am I wrong? This doesn't seem important to me, it does not change any functionality or fix a problem. But it seems simple enough so I can take it. It seems just solving nits from comment 5 is needed here.
Assignee: hiikezoe → acelists
Status: NEW → ASSIGNED
Comment 12•9 years ago
|
||
(In reply to Kent James (:rkent) from comment #8) > I'm not much of a file system expert, but it is not clear to me that stat is > a real improvement over Exists. Maybe Chiaki has some thoughts? Sorry, I missed the boat, so to speak. I think this change has more to do with the principle of using portable library instead of invoking bare system calls in the main code. As far as I can tell, TB and FF code adheres to this very well. Most developers won't bother even if Exists() eventually is implemented by |stat| under POSIX OS, or access. (Seeing stat() in the main code is like seeing unwrapped |errno| being accessed directly in high-level main code. Sometimes I wish I could, but knowing that semantics of |errno| may not work well in Windows binary, I have to go through the portable library's error handling framework, which I feel does not seem to address error handling very well.) I doubt if there is much performance difference between stat() and access(). They are used for different purposes to begin with. For example, if, say, one checks the access right of a file/directory based on the access bits returned by stat() by a series of if (bit-manipulation-expression), then using access() in the first place is straight-forward and appropriate. But of course, access() doesn't give you all the detailed information which stat() returns. If Exists() only checks for existence, I think stat() is just fine as access(). The difference of performance would probably be important only when you need to check for 10,000 files. Re cache: However, if |Exists| uses its own idea of caching of "known" files/folder (see Comment 1: "There's a known bug on windows that Exists can cache the status of the file producing an out of date result. So I think we either need to clone the nsIFile (which we've done in other places) or keep passing the string and create a new nsIFile for this Exists test."), then there *may* be performance improvement. However, the windows bug needs to be avoided, if it is still there. Also, I tend to add/modify/delete files (folder as well as .msf files) during tests, and such out-of-band file modifications may need to be handled properly by such "cache" mechanism. At least, TB should not crash even if file is removed while TB is not running. But this caching issue with its own bug is orthogonal to this bug entry, and so it is a good idea to remove the mention of "stat" in the code. TIA
Flags: needinfo?(ishikawa)
Assignee | ||
Comment 13•9 years ago
|
||
This should solve requests from standard8. Aryx, please push to try server.
Attachment #8374509 -
Attachment is obsolete: true
Flags: needinfo?(archaeopteryx)
Attachment #8596718 -
Attachment filename: nsfile → 786141.patch
Attachment #8596718 -
Attachment is patch: true
Comment 14•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=73dedd443b47
Flags: needinfo?(archaeopteryx)
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8596718 [details] [diff] [review] updated patch Thanks, I do not see any new failures in that try run.
Attachment #8596718 -
Flags: review?(neil)
Comment 16•9 years ago
|
||
Comment on attachment 8596718 [details] [diff] [review] updated patch > m_dbName = dbName; >- if (stat(dbName, &st)) >+ bool exists = false; >+ nsCOMPtr<nsIFile> dbFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &ret); >+ if (NS_SUCCEEDED(ret) && dbFile) { >+ ret = dbFile->InitWithNativePath(nsDependentCString(dbName)); Nit: Use m_dbName, it's already an nsCString.
Attachment #8596718 -
Flags: review?(neil) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Thanks.
Attachment #8596718 -
Attachment is obsolete: true
Attachment #8597745 -
Flags: review+
Keywords: checkin-needed
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Comment 19•9 years ago
|
||
Sorry for the incorrect "/suite part" appended to the commit message by mistake.
You need to log in
before you can comment on or make changes to this bug.
Description
•