Free BerkeleyEnvironment instances when not in use #11911

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/countenv changing 5 files +124 −33
  1. ryanofsky commented at 7:18 pm on December 15, 2017: member

    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.

  2. ryanofsky force-pushed on Dec 15, 2017
  3. fanquake added the label Wallet on Dec 15, 2017
  4. in src/wallet/db.h:90 in aca8ec0dab outdated 
    86@@ -86,7 +87,7 @@ class CDBEnv
87 };
88 
89 /** Get CDBEnv and database filename given a wallet path. */
90-CDBEnv* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename);
91+std::shared_ptr<CDBEnv> GetWalletEnv(const fs::path& wallet_path, std::string& database_filename);
    promag commented at 2:54 am on December 18, 2017:
    typedef std::shared_ptr<CDBEnv> something? CDBEnvRef for instance.
    ryanofsky commented at 4:31 pm on December 18, 2017:

    typedef std::shared_ptr something? CDBEnvRef for instance

    I don’t think this is a good idea. I’d be reluctant to introduce yet another wallet database type, especially one that obfuscates what it represents and how it relates to CDBEnv. Use of ref type aliases has been justified in other places on the basis that where types are widely used, having aliases makes it easier to switch implementations without having to update a lot of existing code. But that justification doesn’t apply here because CDBEnv is internal to db.h/db.cpp and not widely used.

    promag commented at 8:21 pm on December 18, 2017:
    :+1:
  5. ryanofsky force-pushed on Dec 21, 2017
  6. ryanofsky force-pushed on Dec 21, 2017
  7. ryanofsky force-pushed on Dec 21, 2017
  8. ryanofsky force-pushed on Dec 31, 2017
  9. ryanofsky force-pushed on Jan 11, 2018
  10. ryanofsky force-pushed on Jan 12, 2018
  11. ryanofsky force-pushed on Jan 16, 2018
  12. ryanofsky force-pushed on Jan 22, 2018
  13. ryanofsky force-pushed on Jan 25, 2018
  14. ryanofsky force-pushed on Feb 5, 2018
  15. ryanofsky force-pushed on Feb 23, 2018
  16. ryanofsky force-pushed on Mar 2, 2018
  17. ryanofsky force-pushed on Mar 7, 2018
  18. ryanofsky force-pushed on Apr 10, 2018
  19. in src/wallet/db.cpp:150 in e4a5ea35fb outdated 
    123@@ -121,6 +124,7 @@ BerkeleyEnvironment::BerkeleyEnvironment(const fs::path& dir_path) : strPath(dir
124 
125 BerkeleyEnvironment::~BerkeleyEnvironment()
126 {
127+    g_dbenvs.erase(strPath);
    promag commented at 10:08 pm on May 2, 2018:
    Missing cs_db lock?
    promag commented at 10:13 pm on May 2, 2018:
    Could you change to std::string const strPath; in BerkeleyEnvironment?
    promag commented at 10:15 pm on May 2, 2018:

    Nit:

    0int count = g_dbenvs.erase(strPath);
1assert(count == 1);
  20. in src/wallet/db.cpp:88 in e4a5ea35fb outdated 
    81+    if (inserted.second) {
82+        auto env = std::make_shared<BerkeleyEnvironment>(env_directory.string());
83+        inserted.first->second = env;
84+        return env;
85+    } else {
86+        return inserted.first->second.lock();
    sipa commented at 11:11 pm on May 2, 2018:
    I may be missing the logic here, but how is it guaranteed that the shared_ptr hasn’t expired yet?
    promag commented at 11:17 pm on May 2, 2018:
    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.
    sipa commented at 11:29 pm on May 2, 2018:
    Ah, of course, makes sense.
  21. sipa commented at 11:35 pm on May 2, 2018: member
    Concept ACK
  22. ryanofsky force-pushed on May 14, 2018
  23. ryanofsky force-pushed on May 18, 2018
  24. promag commented at 10:01 pm on July 8, 2018: member
    @ryanofsky ping.
  25. DrahtBot closed this on Jul 22, 2018
  26. DrahtBot commented at 11:50 pm on July 22, 2018: member
  27. DrahtBot reopened this on Jul 22, 2018
  28. DrahtBot added the label Needs rebase on Aug 9, 2018
  29. ryanofsky force-pushed on Aug 9, 2018
  30. DrahtBot removed the label Needs rebase on Aug 9, 2018
  31. ryanofsky force-pushed on Sep 4, 2018
  32. DrahtBot added the label Needs rebase on Sep 5, 2018
  33. ryanofsky force-pushed on Sep 5, 2018
  34. DrahtBot removed the label Needs rebase on Sep 6, 2018
  35. in src/wallet/db.cpp:517 in a35b4ef797 outdated 
    506@@ -504,7 +507,7 @@ BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bo
507             // versions of BDB have an set_lk_exclusive method for this
508             // purpose, but the older version we use does not.)
509             for (const auto& env : g_dbenvs) {
510-                CheckUniqueFileid(env.second, strFilename, *pdb_temp);
511+                CheckUniqueFileid(*env.second.lock().get(), strFilename, *pdb_temp);
    PierreRochard commented at 8:08 pm on September 14, 2018:
    Pre-dates this commit, but env variable name shadowing here threw me for a loop
  36. PierreRochard approved
  37. PierreRochard commented at 1:14 am on September 15, 2018: contributor

    Tested ACK a35b4ef797783596dff9ca24fc46789a84e90014

    If it’s useful I wrote a few unit tests https://github.com/PierreRochard/bitcoin/commit/fc47a8843b03718529c402fd42cabb7a0834a9ff and code comments https://github.com/PierreRochard/bitcoin/commit/ea7f8291dab051cceb2751ef40f7d2a0ee67f95c

  38. PierreRochard commented at 1:43 pm on September 15, 2018: contributor
    One more thing: PR title and commit message should be updated to say BerkeleyEnvironment
  39. DrahtBot commented at 1:29 pm on September 21, 2018: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    No conflicts as of last run.

  40. ryanofsky force-pushed on Sep 26, 2018
  41. ryanofsky renamed this:
    Free CDBEnv instances when not in use
    Free BerkeleyEnvironment instances when not in use
    on Sep 26, 2018
  42. in src/wallet/db.cpp:87 in 3fff7065b5 outdated 
    87+    auto inserted = g_dbenvs.emplace(env_directory.string(), std::weak_ptr<BerkeleyEnvironment>());
88+    if (inserted.second) {
89+        auto env = std::make_shared<BerkeleyEnvironment>(env_directory.string());
90+        inserted.first->second = env;
91+        return env;
92+    } else {
    practicalswift commented at 7:58 am on October 15, 2018:
    Drop this redundant else and move the return the outer scope?
  43. ryanofsky force-pushed on Oct 15, 2018
  44. DrahtBot added the label Needs rebase on Oct 20, 2018
  45. ryanofsky force-pushed on Oct 22, 2018
  46. DrahtBot removed the label Needs rebase on Oct 22, 2018
  47. in src/wallet/db.cpp:100 in 54d2a8ef1f outdated 
    83-    // emplace function if the key already exists. This is a little inefficient,
84-    // but not a big concern since the map will be changed in the future to hold
85-    // pointers instead of objects, anyway.
86-    return &g_dbenvs.emplace(std::piecewise_construct, std::forward_as_tuple(env_directory.string()), std::forward_as_tuple(env_directory)).first->second;
87+    auto inserted = g_dbenvs.emplace(env_directory.string(), std::weak_ptr<BerkeleyEnvironment>());
88+    if (inserted.second) {
    promag commented at 10:02 am on October 24, 2018:

    I think this condition should be replaced with:

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

    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.

  48. DrahtBot added the label Needs rebase on Oct 24, 2018
  49. ryanofsky force-pushed on Oct 25, 2018
  50. DrahtBot removed the label Needs rebase on Oct 25, 2018
  51. in src/wallet/wallet.cpp:3881 in 119ceb458a outdated 
    3850@@ -3851,6 +3851,9 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string&
3851         }
3852     }
3853 
3854+    // Keep same database environment instance across Verify/Recover calls below.
3855+    std::unique_ptr<WalletDatabase> database = WalletDatabase::Create(wallet_path);
    practicalswift commented at 8:07 am on November 13, 2018:
    This declaration hides the member CWallet::database. Is that intentional? If not, use another name? :-)
  52. meshcollider commented at 3:56 am on November 15, 2018: contributor
    utACK https://github.com/bitcoin/bitcoin/pull/11911/commits/530efe9813fc1fc874c7b612cfd46cc00beb4bf3 modulo @promag’s comments above, which need addressing
  53. DrahtBot added the label Needs rebase on Nov 20, 2018
  54. 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.
    f1f4bb7345
  55. Tests: add unit tests for GetWalletEnv 88b1d956fe
  56. Trivial: add doxygen-compatible comments relating to BerkeleyEnvironment 14bc2a17dd
  57. ryanofsky force-pushed on Nov 26, 2018
  58. DrahtBot removed the label Needs rebase on Nov 27, 2018
  59. ryanofsky force-pushed on Nov 27, 2018
  60. ken2812221 approved
  61. ken2812221 commented at 3:56 am on November 28, 2018: contributor
    utACK 14bc2a17dd03ccd89f65a302328763ff22c710c2. Could remove FUNCTIONAL_TESTS_CONFIG="--exclude wallet_multiwallet.py" from travis option
  62. practicalswift commented at 7:00 am on November 28, 2018: contributor
    @ryanofsky Can you try without FUNCTIONAL_TESTS_CONFIG? :-)
  63. MarcoFalke added this to the milestone 0.18.0 on Dec 3, 2018
  64. MarcoFalke commented at 4:55 pm on January 15, 2019: member
    @ryanofsky Are you still working on this?
  65. ryanofsky commented at 3:49 pm on January 16, 2019: member

    @ryanofsky Are you still working on this?

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

  66. promag commented at 0:35 am on January 31, 2019: member

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

     0--- a/src/wallet/db.cpp
 1+++ b/src/wallet/db.cpp
 2@@ -97,8 +97,9 @@ std::shared_ptr<BerkeleyEnvironment> GetWalletEnv(const fs::path& wallet_path, s
 3     SplitWalletPath(wallet_path, env_directory, database_filename);
 4     LOCK(cs_db);
 5     auto inserted = g_dbenvs.emplace(env_directory.string(), std::weak_ptr<BerkeleyEnvironment>());
 6-    if (inserted.second) {
 7-        auto env = std::make_shared<BerkeleyEnvironment>(env_directory.string());
 8+    auto env = inserted.first->second.lock();
 9+    if (!env) {
10+        env = std::make_shared<BerkeleyEnvironment>(env_directory.string());
11         inserted.first->second = env;
12         return env;
13     }
  67. laanwj commented at 5:00 pm on January 31, 2019: member
    utACK 14bc2a17dd03ccd89f65a302328763ff22c710c2
  68. laanwj merged this on Jan 31, 2019
  69. laanwj closed this on Jan 31, 2019
  70. laanwj referenced this in commit efb6ddef9c on Jan 31, 2019
  71. promag referenced this in commit f22d02f537 on Mar 12, 2019
  72. promag referenced this in commit 85c6263ddb on Mar 12, 2019
  73. promag referenced this in commit f20513bd71 on Mar 12, 2019
  74. laanwj referenced this in commit 6cf81b01b4 on Mar 20, 2019
  75. sidhujag referenced this in commit 1268aa7a4e on Mar 28, 2019
  76. sidhujag referenced this in commit 838e86656e on Mar 28, 2019
  77. sidhujag referenced this in commit 51d49e7a7e on Mar 28, 2019
  78. sidhujag referenced this in commit 3ee64a0aa7 on Mar 28, 2019
  79. uhliksk referenced this in commit 22975a997a on Apr 21, 2019
  80. uhliksk referenced this in commit 104e930d3f on Apr 21, 2019
  81. uhliksk referenced this in commit d973ab0c2e on Apr 21, 2019
  82. Rishabh42 referenced this in commit ce3547fae6 on Apr 22, 2019
  83. uhliksk referenced this in commit 3da68df9e3 on May 1, 2019
  84. uhliksk referenced this in commit e1ec67ad34 on May 1, 2019
  85. uhliksk referenced this in commit c1f7d57d62 on May 1, 2019
  86. jasonbcox referenced this in commit 8915b2f748 on Oct 25, 2019
  87. UdjinM6 referenced this in commit d4e05c5e19 on Jul 15, 2021
  88. UdjinM6 referenced this in commit 82ba524a3f on Jul 15, 2021
  89. UdjinM6 referenced this in commit c555352bc7 on Jul 15, 2021
  90. UdjinM6 referenced this in commit e681ffc3bb on Jul 15, 2021
  91. UdjinM6 referenced this in commit 85b8a05690 on Jul 16, 2021
  92. DrahtBot locked this on Dec 16, 2021


ryanofsky promag sipa DrahtBot PierreRochard practicalswift meshcollider ken2812221 MarcoFalke laanwj

Labels
Wallet

Milestone
0.18.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me