util: remove DataDir caching

laanwj commented at 12:52 pm on October 9, 2013: member

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/csPathCached destruction order problems (if they are still around).

util: remove DataDir caching 23e0d45b79

in src/util.cpp: in 4f17086c93 outdated

1032@@ -1033,9 +1033,7 @@ void PrintExceptionContinue(std::exception* pex, const char* pszThread)
1033         pathRet = fs::path(pszHome);
1034 #ifdef MAC_OSX
1035     // Mac
1036-    pathRet /= "Library/Application Support";
1037-    fs::create_directory(pathRet);

laanwj commented at 12:53 pm on October 9, 2013:

Removed this fs::create_directory as we do fs::create_directories anyway which creates all parent directories as needed.

Diapolo commented at 7:10 am on October 10, 2013:

You mean in GetDataDir()?

laanwj commented at 7:13 am on October 10, 2013:

Yes. Or are there any other paths to here?

Diapolo commented at 10:21 am on October 11, 2013:

AFAIK no.

Diapolo commented at 7:11 am on October 10, 2013: none

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.

BitcoinPullTester commented at 5:24 am on October 11, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/23e0d45b79f1885a318f9eed6a7c5ceb6e449206 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

in src/util.cpp: in 23e0d45b79

1040@@ -1043,24 +1041,10 @@ void PrintExceptionContinue(std::exception* pex, const char* pszThread)
1041 #endif
1042 }
1043 
1044-static boost::filesystem::path pathCached[CChainParams::MAX_NETWORK_TYPES+1];
1045-static CCriticalSection csPathCached;
1046-
1047-const boost::filesystem::path &GetDataDir(bool fNetSpecific)
1048+const boost::filesystem::path GetDataDir(bool fNetSpecific)

sipa commented at 8:31 pm on October 11, 2013:

Returning a const by value doesn’t mean much :)

laanwj commented at 8:49 pm on October 11, 2013:

Lol, indeed, I’ll remove the const

sipa commented at 8:34 pm on October 13, 2013: member

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.

laanwj commented at 6:33 am on October 15, 2013: member

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 Params().DataDir().

laanwj commented at 10:24 am on October 19, 2013: member

I don’t care enough about this to keep it open.

laanwj closed this on Oct 19, 2013

laanwj referenced this in commit d080a7d503 on Nov 18, 2017

Bushstar referenced this in commit 2c7b29bac5 on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021

util: remove DataDir caching #3073