Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

get_producers unittest #9923

Merged
merged 8 commits into from
Jan 20, 2021
Merged

Conversation

huangminghuang
Copy link
Contributor

@huangminghuang huangminghuang commented Jan 18, 2021

Change Description

EPE-582
This PR

  • fixes a problem to add table row with f64 secondary key.
  • add a unit test for get_producers

Change Type

Select ONE:

  • Documentation
  • Stability bug fix
  • Other
  • Other - special case

Testing Changes

Select ANY that apply:

  • New Tests
  • Existing Tests
  • Test Framework
  • CI System
  • Other

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

@brianjohnson5972
Copy link
Contributor

Adding tests and further fixes


eosio::session::shared_bytes create_full_primary_key(const eosio::session::shared_bytes& prefix, uint64_t primary_key) {
const auto prefix_size = detail::prefix_size<eosio::session::shared_bytes>();
b1::chain_kv::bytes composite_key;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you need dynamic memory allocation here. I would use something like the following to eliminate the dynamic memory and branches.

char composite_key[sizeof(prmary_key) + type_size] = { static_cast<char>(key_type::primary) } ;
int prerix_contains_key_type =  (prefix.size() == prefix_size + type_size);
EOS_ASSERT(prefix.size() == prefix_size || ( prerix_contains_key_type &&   static_cast<key_type>(prefix[prefix.size() - 1]) == key_type::primary_key), ....);
memcpy(composite_key + type_size, &primary_key, sizeof(prmary_key)); 
return eosio::session::make_shared_bytes<std::string_view, 2>(
        {  std::string_view{prefix.data(),  prefix.size()},
            std::string_view{composite_key + prerix_contains_key_type, 
                                         sizeof(composite_key) - prerix_contains_key_type} });

Copy link
Contributor

@brianjohnson5972 brianjohnson5972 Jan 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for needing the dynamic memory is that initially all this code was written around chain_kv code. Since the new rocksdb session code was added afterward, the decision was to take that existing code and massage it to the new format (with the intrinsic type char and contract name at the beginning). This code was designed to prevent constructing the whole key from scratch, since they would all have the same prefix. All this code definitely needs to be refactored and cleaned up, and there are a lot of copied code, but at least it has the same pattern of code. I think it would be better to remove this method and just construct the key from scratch rather than have a highly optimized method that is different than other functions performing similar actions, and this is only being used for chain_plugin RPC calls, so not an highly optimized path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, code without branches is easier to read. I am OK if you insisted on the existing code.

@brianjohnson5972 brianjohnson5972 merged commit 438b625 into develop Jan 20, 2021
@brianjohnson5972 brianjohnson5972 deleted the huangminghuang/get-producers-fix branch January 27, 2021 19:47
@nickjjzhao nickjjzhao mentioned this pull request Jan 19, 2022
10 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants