util: remove DataDir caching #3073
Conversation
| @@ -1033,9 +1033,7 @@ void PrintExceptionContinue(std::exception* pex, const char* pszThread) | |||
| pathRet = fs::path(pszHome); | |||
| #ifdef MAC_OSX | |||
| // Mac | |||
| pathRet /= "Library/Application Support"; | |||
| fs::create_directory(pathRet); | |||
There was a problem hiding this comment.
Removed this fs::create_directory as we do fs::create_directories anyway which creates all parent directories as needed.
There was a problem hiding this comment.
Yes. Or are there any other paths to here?
|
I'm fine with that change, I remember it was a pain to see some issues raised, because of wrong data-dir paths because of caching etc. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/23e0d45b79f1885a318f9eed6a7c5ceb6e449206 for binaries and test log. |
| static CCriticalSection csPathCached; | ||
|
|
||
| const boost::filesystem::path &GetDataDir(bool fNetSpecific) | ||
| const boost::filesystem::path GetDataDir(bool fNetSpecific) |
There was a problem hiding this comment.
Returning a const by value doesn't mean much :)
There was a problem hiding this comment.
Lol, indeed, I'll remove the const
|
OpenDiskFile() calls GetDataDir, so during IBD and initial startup it is called many times. It's unlikely to be a performance burden, but it seems silly to make that many system calls (yes, I realize we call open() and read() and gettimeofdate() much more...) over and over again, trusting that it will never change. I'd be fine with just storing the block directory once in main, and reusing that, instead of having a cache at the GetDataDir level. |
|
I think it's silly to use this kind of caching logic for what is a simple string operation that is only invoked before opening a file. With the system call you mean the fs::create_directories (as getenv is not a system call)? I agree that's a bit silly to do every time. It would be better to create the directories at some sane point in the initialization process and not every time before returning. Hmmm maybe the directory could be stored at that point too, avoiding the computation in GetDataDir completely AND any caching logic, and also the dependency on |
|
I don't care enough about this to keep it open. |
c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes #11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in #11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. #3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d
…param c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes bitcoin#11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in bitcoin#11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. bitcoin#3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d
This pull may be slightly controversial, but I dare say that
GetDataDir()is never used on any performance critical paths (and if it is, it is a bug).It's not that heavy in the first place.
This change simplifies the code (no more ClearDatadirCache) and removes any
pathCached/csPathCacheddestruction order problems (if they are still around).