Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Free BerkeleyEnvironment instances when not in use #11911

Merged
merged 3 commits into from Jan 31, 2019

Conversation

Projects
None yet
@ryanofsky
Copy link
Contributor

ryanofsky commented Dec 15, 2017

Instead of adding BerkeleyEnvironment objects permanently to the g_dbenvs map, use reference counted shared pointers and remove map entries when the last BerkeleyEnvironment reference goes out of scope.

This change was requested by @TheBlueMatt and makes code that sets up mock databases cleaner. The mock database environment will now go out of scope and be reset on destruction so there is no need to call BerkeleyEnvironment::Reset() during wallet construction to clear out prior state.

This change does affect bitcoin behavior slightly. On startup, instead of same wallet environments staying open throughout VerifyWallets() and OpenWallets() calls, VerifyWallets() will open and close an environment once for each wallet, and OpenWallets() will create its own environment(s) later.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch from 7f3cb3d to aca8ec0 Dec 15, 2017

@fanquake fanquake added the Wallet label Dec 15, 2017

Show resolved Hide resolved src/wallet/db.h Outdated

@ryanofsky ryanofsky referenced this pull request Dec 18, 2017

Merged

External wallet files #11687

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch 3 times, most recently to 65ce46b Dec 21, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch from 65ce46b to cd44f30 Dec 31, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch 3 times, most recently from fdb443a to c5c0687 Jan 11, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch 2 times, most recently from e7c003e to da2e3ea Jan 22, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch from da2e3ea to f0e5893 Feb 5, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch from f0e5893 to 702f8b5 Feb 23, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch 2 times, most recently from 4f2e03d to 3ffbabd Mar 2, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch from 3ffbabd to e4a5ea3 Apr 10, 2018

@@ -121,6 +124,7 @@ BerkeleyEnvironment::BerkeleyEnvironment(const fs::path& dir_path) : strPath(dir

BerkeleyEnvironment::~BerkeleyEnvironment()
{
g_dbenvs.erase(strPath);

This comment has been minimized.

@promag

promag May 2, 2018

Member

Missing cs_db lock?

This comment has been minimized.

@promag

promag May 2, 2018

Member

Could you change to std::string const strPath; in BerkeleyEnvironment?

This comment has been minimized.

@promag

promag May 2, 2018

Member

Nit:

int count = g_dbenvs.erase(strPath);
assert(count == 1);
inserted.first->second = env;
return env;
} else {
return inserted.first->second.lock();

This comment has been minimized.

@sipa

sipa May 2, 2018

Member

I may be missing the logic here, but how is it guaranteed that the shared_ptr hasn't expired yet?

This comment has been minimized.

@promag

promag May 2, 2018

Member

It's valid until ~BerkeleyEnvironment() which means shared_ptr just expired, and in ~BerkeleyEnvironment() the map is erased. In other words, when shared_ptr releases the instance the entry in g_dbenvs is removed.

This comment has been minimized.

@sipa

sipa May 2, 2018

Member

Ah, of course, makes sense.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 2, 2018

Concept ACK

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch 2 times, most recently from 0e2b276 to 09813c5 May 14, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented Jul 8, 2018

@ryanofsky ping.

@DrahtBot DrahtBot closed this Jul 22, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jul 22, 2018

The last travis run for this pull request was 65 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
// pointers instead of objects, anyway.
return &g_dbenvs.emplace(std::piecewise_construct, std::forward_as_tuple(env_directory.string()), std::forward_as_tuple(env_directory)).first->second;
auto inserted = g_dbenvs.emplace(env_directory.string(), std::weak_ptr<BerkeleyEnvironment>());
if (inserted.second) {

This comment has been minimized.

@promag

promag Oct 24, 2018

Member

I think this condition should be replaced with:

auto env = inserted.first->second.lock();
if (!env) {
    // ...

which prevents returning null std::shared_ptr<BerkeleyEnvironment>.

Note that BerkeleyEnvironment destructor could be called after the above LOCK(cs_db) which means it would returng an invalid env.

@@ -3851,6 +3851,9 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string&
}
}

// Keep same database environment instance across Verify/Recover calls below.
std::unique_ptr<WalletDatabase> database = WalletDatabase::Create(wallet_path);

This comment has been minimized.

@practicalswift

practicalswift Nov 13, 2018

Member

This declaration hides the member CWallet::database. Is that intentional? If not, use another name? :-)

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Nov 15, 2018

utACK 530efe9 modulo @promag's comments above, which need addressing

ryanofsky and others added some commits May 18, 2018

Free BerkeleyEnvironment instances when not in use
Instead of adding BerkeleyEnvironment objects permanently to the g_dbenvs map,
use reference counted shared pointers and remove map entries when the last
BerkeleyEnvironment reference goes out of scope.

This change was requested by Matt Corallo <git@bluematt.me> and makes code that
sets up mock databases cleaner. The mock database environment will now go out
of scope and be reset on destruction so there is no need to call
BerkeleyEnvironment::Reset() during wallet construction to clear out prior
state.

This change does affect bitcoin behavior slightly. On startup, instead of same
wallet environments staying open throughout VerifyWallets() and OpenWallets()
calls, VerifyWallets() will open and close an environment once for each wallet,
and OpenWallets() will create its own environment(s) later.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch from 119ceb4 to 6a66d3d Nov 26, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 27, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/countenv branch from 6a66d3d to 14bc2a1 Nov 27, 2018

@ken2812221
Copy link
Member

ken2812221 left a comment

utACK 14bc2a1. Could remove FUNCTIONAL_TESTS_CONFIG="--exclude wallet_multiwallet.py" from travis option

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 28, 2018

@ryanofsky Can you try without FUNCTIONAL_TESTS_CONFIG? :-)

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Dec 3, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jan 15, 2019

@ryanofsky Are you still working on this?

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Jan 16, 2019

@ryanofsky Are you still working on this?

Yeah, I went back and implemented all the suggestions, but for some reason the assert in #11911 (comment) is constantly triggering, so I need to debug.

@promag

This comment has been minimized.

Copy link
Member

promag commented Jan 31, 2019

Tested ACK 14bc2a1 while implementing #15297. Also tested with

--- a/src/wallet/db.cpp
+++ b/src/wallet/db.cpp
@@ -97,8 +97,9 @@ std::shared_ptr<BerkeleyEnvironment> GetWalletEnv(const fs::path& wallet_path, s
     SplitWalletPath(wallet_path, env_directory, database_filename);
     LOCK(cs_db);
     auto inserted = g_dbenvs.emplace(env_directory.string(), std::weak_ptr<BerkeleyEnvironment>());
-    if (inserted.second) {
-        auto env = std::make_shared<BerkeleyEnvironment>(env_directory.string());
+    auto env = inserted.first->second.lock();
+    if (!env) {
+        env = std::make_shared<BerkeleyEnvironment>(env_directory.string());
         inserted.first->second = env;
         return env;
     }
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 31, 2019

utACK 14bc2a1

@laanwj laanwj merged commit 14bc2a1 into bitcoin:master Jan 31, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 31, 2019

Merge #11911: Free BerkeleyEnvironment instances when not in use
14bc2a1 Trivial: add doxygen-compatible comments relating to BerkeleyEnvironment (Pierre Rochard)
88b1d95 Tests: add unit tests for GetWalletEnv (Pierre Rochard)
f1f4bb7 Free BerkeleyEnvironment instances when not in use (Russell Yanofsky)

Pull request description:

  Instead of adding BerkeleyEnvironment objects permanently to the g_dbenvs map, use reference counted shared pointers and remove map entries when the last BerkeleyEnvironment reference goes out of scope.

  This change was requested by @TheBlueMatt and makes code that sets up mock databases cleaner. The mock database environment will now go out of scope and be reset on destruction so there is no need to call BerkeleyEnvironment::Reset() during wallet construction to clear out prior state.

  This change does affect bitcoin behavior slightly. On startup, instead of same wallet environments staying open throughout VerifyWallets() and OpenWallets() calls, VerifyWallets() will open and close an environment once for each wallet, and OpenWallets() will create its own environment(s) later.

Tree-SHA512: 219d77a9e2268298435b86088f998795e059fdab1d2050ba284a9ab8d8a44961c9b5cf96e94ee521688108d23c6db680e3e3a999b8cb2ac2a8590f691d50668b

promag added a commit to promag/bitcoin that referenced this pull request Mar 12, 2019

Free BerkeleyEnvironment instances when not in use
Instead of adding BerkeleyEnvironment objects permanently to the g_dbenvs map,
use reference counted shared pointers and remove map entries when the last
BerkeleyEnvironment reference goes out of scope.

This change was requested by Matt Corallo <git@bluematt.me> and makes code that
sets up mock databases cleaner. The mock database environment will now go out
of scope and be reset on destruction so there is no need to call
BerkeleyEnvironment::Reset() during wallet construction to clear out prior
state.

This change does affect bitcoin behavior slightly. On startup, instead of same
wallet environments staying open throughout VerifyWallets() and OpenWallets()
calls, VerifyWallets() will open and close an environment once for each wallet,
and OpenWallets() will create its own environment(s) later.

Github-Pull: bitcoin#11911
Rebased-From: f1f4bb7

promag added a commit to promag/bitcoin that referenced this pull request Mar 12, 2019

promag added a commit to promag/bitcoin that referenced this pull request Mar 12, 2019

@promag promag referenced this pull request Mar 12, 2019

Open

0.17: Backport 15297 #15575

LitecoinZ added a commit to litecoinz-core/litecoinz that referenced this pull request Mar 17, 2019

Backport bitcoin#15297
- bitcoin#12493 [wallet] Reopen CDBEnv after encryption instead of shutting down
- bitcoin#14350 Add WalletLocation class
- bitcoin#14320 [bugfix] wallet: Fix duplicate fileid detection
- bitcoin#14552 wallet: detecting duplicate wallet by comparing the db filename.
- bitcoin#11911 Free BerkeleyEnvironment instances when not in use
- bitcoin#15297 wallet: Releases dangling files on BerkeleyEnvironment::Close
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.