mastercoin-MSC / mastercore

mastercore info
mastercoin.org
MIT License
24 stars 11 forks source link

Replace global balance arrays with std::map #258

Closed dexX7 closed 9 years ago

dexX7 commented 9 years ago
  1. Doesn't cover full range of potential properties, can result in out-of-bounds violation
  2. No need to reserve 100000 elements each
  3. [] operator reads an element at position, if key exists, or inserts an element, if not

Build result: https://travis-ci.org/dexX7/mastercore/builds/44842876

m21 commented 9 years ago

@zathras-crypto to review, perhaps not for the immediate tag

zathras-crypto commented 9 years ago

Like the direction. Please hold for now, would also need modifications to UI code where those globals are queried. To be looked at after tag.

Also if moving to map:

+std::map<uint32_t, uint64_t> global_balance_money_testeco;
+std::map<uint32_t, uint64_t> global_balance_reserved_testeco;

are redundant, uint32_t can accommodate both main and test ecosystem property ID's so a single map for money and reserved should suffice.

FYI they were only initially separate so I could init the arrays at low values; one big uint64_t someArray [max_uint32] array would have been a silly amount of memory and figured would be well into future versions and using a better approach before we had anything close to 100K properties in either eco).

TL:DR; seems @dexX7 has basically addressed for me:

//temp - only supporting 100,000 properties per eco here, research best way to expand array
//these 4 arrays use about 3MB total memory with 100K properties limit (100000*8*4 bytes)

Which is awesome and pleased with approach :), but suggest wait until after tag to add in UI bits affected by this (may even be good opportunity for me to do the SP list in overview as a proper listwidget instead of 5 sets of labels).

Thanks Z

zathras-crypto commented 9 years ago

Sorry, missed

[] operator reads an element at position, if key exists, or inserts an element, if not

So there actually shouldn't be much if any change on the UI code :)

dexX7 commented 9 years ago

There is no need to change anything, because a map has similar properties in this context (thus possible to change only 8 lines), but I agree, merging money_testeco and reserved_testeco would certainly make sense.

Next up: merge, then replace those loops in sendmpdialog.cpp#L85-L111 etc., and use an iterator instead, e.g. something like:

for (std::map<uint32_t, uint64_t>::const_iterator it = total_money.begin(); it != total_money.end(); ++it) {
    // populate property selector
}

But one step at the time. You almost dismissed this PR already. :P

m21 commented 9 years ago

So this is for past the immediate STO tag, UI-related or TBD differently.

dexX7 commented 9 years ago

@m21: Huh? This is not UI related, but replaces the static arrays used to temporary store total token amounts by dynamic std::maps, which have similar properties, but dynamic allocation and most importantly: 100000 does not cover the full range of possible property ids.

-uint64_t global_balance_money_maineco[100000];
-uint64_t global_balance_reserved_maineco[100000];
-uint64_t global_balance_money_testeco[100000];
-uint64_t global_balance_reserved_testeco[100000];
+std::map<uint32_t, uint64_t> global_balance_money_maineco;
+std::map<uint32_t, uint64_t> global_balance_reserved_maineco;
+std::map<uint32_t, uint64_t> global_balance_money_testeco;
+std::map<uint32_t, uint64_t> global_balance_reserved_testeco;

@zathras-crypto mentioned test and main ecosystem could be merged into one map, where I agree, but one step at the time. And once that's done, the UI code can be reduced which uses the array/map.

m21 commented 9 years ago

The static arrays are there purely for UI's reasons. Up to @zathras-crypto when this should go in, I propose after the immediate tag.

dexX7 commented 9 years ago

This was adopted via https://github.com/zathras-crypto/omnicore/pull/67.