Refactored wallet.cpp file because was too big (issue #950) #1413

cogutvalera · 2018-11-01T08:35:06Z

This PR for #950 issue wallet.cpp file too big

jmjatlanta · 2018-11-01T13:03:43Z

Ironically, this change led my compiler to memory exhaustion. Previous builds were close, this build pushed it over the edge. 7 processors, 16GB RAM, 6GB Swap. Recompiling with fewer processors got it to compile. cli_test and chain_test ran without errors. Now reviewing the code.

cogutvalera · 2018-11-01T13:19:54Z

@jmjatlanta Thank you very much !

cogutvalera · 2018-11-01T13:21:45Z

@jmjatlanta FYI if you will try to start cli_wallet without merging this PR bitshares/bitshares-fc#86 then you will catch SEGFAULT

jmjatlanta

Need to move 1 method, and looking for comments from others.

libraries/wallet/implementation/wallet_network_impl.cpp

jmjatlanta · 2018-11-05T11:12:31Z

libraries/wallet/wallet.cpp

@@ -79,163 +80,27 @@
 # include <sys/stat.h>
 #endif

+#include "wallet_account.cpp"


Need to be careful here. If make compiles these as objects (i.e. cmake tells it to), linkers will not be able to link.

This is certainly a way to fix the issue reported, and the precompiler will do its job. But I wonder if this is the best way to fix it. Honestly, I would break these into separate classes (ACID principle), but that would require a larger effort. Perhaps @abitmore or @pmconrad or @oxarbitrage have ideas on how it could be done.

Thanks ! My solution requires minor changes just to separate different logical parts in different files like for example database.cpp implementation.

Good, but then please also use the same mechanism as used for database.cpp: https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/CMakeLists.txt#L11-L30 (can use the same constant).

ok sure ! Thanks !

cogutvalera · 2018-11-08T14:27:52Z

Bumped FC library to fix conflicts

cogutvalera · 2018-11-12T06:20:44Z

I've added mechanism as used for database.cpp, so is all fine now ? What are your thoughts guys ?
IMHO we can merge and close this PR and it's related issue, right ?

pmconrad · 2018-11-12T15:29:19Z

Have you tested disabling the unity build? It doesn't compile for me.

pmconrad · 2018-11-12T15:55:32Z

This is a PITA to review.
Next time you tackle such a huge refactoring please do it in small steps in individual commits that are easily verifiable.

cogutvalera · 2018-11-12T16:00:52Z

This is a PITA to review.
Next time you tackle such a huge refactoring please do it in small steps in individual commits that are easily verifiable.

ok sure ! sorry ! Thanks !

cogutvalera · 2018-11-12T16:49:12Z

Have you tested disabling the unity build? It doesn't compile for me.

I've tested with unity build enabled only and all compiled well enough without any errors detected.
Checking now again but with unity build disabled at this time ... give me just a bit time and I will make new commit with fix if there is something wrong. Also do we need case with unity build disabled or maybe should I remove it ?

pmconrad

Still working on the implementation/*cpp files.

pmconrad · 2018-11-12T16:59:46Z

libraries/wallet/wallet_transaction_builder.cpp

+    return my->broadcast_transaction(tx);
+}
+
+signed_transaction wallet_api::sign_transaction(signed_transaction tx, bool broadcast /* = false */)


Please move to general.

pmconrad · 2018-11-12T16:59:51Z

libraries/wallet/wallet_transaction_builder.cpp

+   return fc::to_hex(fc::raw::pack(tx));
+}
+
+pair<transaction_id_type,signed_transaction> wallet_api::broadcast_transaction(signed_transaction tx)


Please move to general.

pmconrad · 2018-11-12T16:59:56Z

libraries/wallet/wallet_transaction_builder.cpp

+   return my->remove_builder_transaction(handle);
+}
+
+string wallet_api::serialize_transaction( signed_transaction tx )const


Please move to general.

pmconrad · 2018-11-12T17:00:32Z

libraries/wallet/wallet_trading.cpp

+   return my->cancel_order(order_id, broadcast);
+}
+
+signed_transaction wallet_api::global_settle_asset(string symbol,


Please move to asset.

pmconrad · 2018-11-12T17:01:07Z

libraries/wallet/wallet_privacy_mode.cpp

@@ -0,0 +1,524 @@
+/*


Please rename to wallet_stealth.cpp

pmconrad · 2018-11-12T17:08:15Z

libraries/wallet/wallet_account.cpp

+   return my->upgrade_account(name,broadcast);
+}
+
+fc::ecc::private_key wallet_api::derive_private_key(const std::string& prefix_string, int sequence_number) const


Please move to general.

pmconrad · 2018-11-12T17:08:48Z

libraries/wallet/wallet_account.cpp

+    return my->_remote_db->get_full_accounts({name_or_id}, false)[name_or_id];
+}
+
+vector<limit_order_object> wallet_api::get_account_limit_orders(


Please move to trading.

pmconrad · 2018-11-12T17:10:26Z

libraries/wallet/wallet_general.cpp

+   return graphene::wallet::utility::suggest_brain_key();
+}
+
+vector<brain_key_info> utility::derive_owner_keys_from_brain_key(string brain_key, int number_of_desired_keys)


IMO this utility method belongs into the _impl file.

pmconrad · 2018-11-12T17:10:32Z

libraries/wallet/wallet_general.cpp

+   return results;
+}
+
+brain_key_info utility::suggest_brain_key()


IMO this utility method belongs into the _impl file.

pmconrad · 2018-11-12T17:21:13Z

libraries/wallet/wallet_transaction_builder.cpp

+ * THE SOFTWARE.
+ */
+
+#include "implementation/wallet_transaction_builder_impl.cpp"


IMO including .cpp files from other .cpp doesn't really fulfill the goal of #950.
Yes, the individual files get smaller, but they are not independent. An IDE must load all included files as well to provide auto-completion for example, so splitting and including doesn't reduce memory requirements. Same for the compiler.

I see you have already partially made this separation. Perhaps it is sufficient to remove these includes and compile the *_impl separately.

pmconrad · 2018-11-12T17:36:15Z

Also do we need case with unity build disabled or maybe should I remove it ?

I have switched to using it by default in my setup, as my impression is that it speeds up the build. Especially when modifying only one of the .cpp files.

No need to add a specific test IMO. Once it's set up correctly it should continue to work.

cogutvalera · 2018-11-12T17:42:04Z

ok, thanks ! soon will make new commits with improvements ...

…on to wallet_general.cpp

cogutvalera · 2018-11-13T07:06:41Z

all improvements done already, also thinking and trying to optimize more compilation memory and time

cogutvalera · 2018-11-13T07:10:51Z

still thinking about more optimizing build time and memory, is it possible to extend estimation for this optimization @ryanRfox ? or maybe should I open another new issue for this optimization task ?

PS: #950 was intended to reduce file size towards smaller files with code, but not to optimize memory and time compilation

Thanks !

cogutvalera · 2018-11-13T10:25:18Z

created new issue for deeper wallet library refactoring #1432

pmconrad · 2018-11-13T21:36:34Z

Confirmed that as of f321f43 code was only moved around. No functional changes.

cogutvalera · 2018-11-14T06:59:28Z

Thank you very much ! Really appreciate all core team's time and efforts !
Now and in future will do only such atomic commits so it would be easier for reviewers to see my changes in step-by-step approach.

Thanks !

cogutvalera · 2018-11-18T04:49:35Z

can we merge it ? so I can start working on #1432 based on this merge.
Or what are your thoughts ? Advise please.
Thanks !

cogutvalera · 2018-11-22T06:04:32Z

Guys what should we do with this PR ? Look please when you will have time.

abitmore · 2019-08-02T13:01:54Z

This branch has conflicts that must be resolved.

abitmore · 2019-09-29T16:32:06Z

Closing in favor of #2013.

cogutvalera mentioned this pull request Nov 1, 2018

wallet.cpp file too big #950

Closed

9 tasks

Refactored wallet.cpp file because was too big

8249db6

cogutvalera force-pushed the issue_950 branch from 7e5f9cf to 8249db6 Compare November 1, 2018 08:38

bump FC to avoid SEGFAULT while starting cli_wallet

68fff30

jmjatlanta suggested changes Nov 5, 2018

View reviewed changes

cogutvalera added 3 commits November 5, 2018 22:47

moved flood_network to debug logical part

ca237ad

Added wallet files in CMake

7026ea8

Fixed conflicts by bump FC library

2e43bd2

pmconrad suggested changes Nov 12, 2018

View reviewed changes

cogutvalera added 7 commits November 12, 2018 20:25

moved sign_transaction, broadcast_transaction and serialize transacti…

5d3d84c

…on to wallet_general.cpp

moved global_settle_asset and settle_asset to wallet_asset.cpp

9fe182d

renamed from privacy_mode to stealth

93753b3

moved get_result_formatters to wallet_general.cpp

caf8c7e

moved derive_private_key to wallet_general.cpp

25d314a

moved get_account_limit_orders to wallet_trading.cpp

ec5bc25

moved utility methods to implementation

ffc5f50

oxarbitrage mentioned this pull request Nov 12, 2018

Define coding style guidelines #1318

Open

34 tasks

improved and fixed compilation

f321f43

cogutvalera mentioned this pull request Nov 13, 2018

Refactor wallet library to be modular with non-dependent classes (optimization of compilation time & memory) #1432

Closed

17 tasks

abitmore added this to the Future Feature Release milestone Aug 2, 2019

abitmore mentioned this pull request Sep 18, 2019

CMake cleanup & MinGW support #1706

Merged

jmjatlanta mentioned this pull request Sep 23, 2019

MinGW build failure #2003

Closed

3 tasks

abitmore closed this Sep 29, 2019

abitmore removed this from the Future Feature Release milestone Oct 7, 2019

Refactored wallet.cpp file because was too big (issue #950) #1413

Refactored wallet.cpp file because was too big (issue #950) #1413

Conversation

cogutvalera commented Nov 1, 2018

jmjatlanta commented Nov 1, 2018 • edited Loading

cogutvalera commented Nov 1, 2018

cogutvalera commented Nov 1, 2018

jmjatlanta left a comment

Choose a reason for hiding this comment

jmjatlanta Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cogutvalera commented Nov 8, 2018

cogutvalera commented Nov 12, 2018

pmconrad commented Nov 12, 2018

pmconrad commented Nov 12, 2018

cogutvalera commented Nov 12, 2018

cogutvalera commented Nov 12, 2018

pmconrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmconrad commented Nov 12, 2018

cogutvalera commented Nov 12, 2018

cogutvalera commented Nov 13, 2018

cogutvalera commented Nov 13, 2018 • edited Loading

cogutvalera commented Nov 13, 2018

pmconrad commented Nov 13, 2018

cogutvalera commented Nov 14, 2018

cogutvalera commented Nov 18, 2018

cogutvalera commented Nov 22, 2018

abitmore commented Aug 2, 2019

abitmore commented Sep 29, 2019

jmjatlanta commented Nov 1, 2018 •

edited

Loading

jmjatlanta Nov 5, 2018 •

edited

Loading

cogutvalera commented Nov 13, 2018 •

edited

Loading