Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
This repository was archived by the owner on Aug 2, 2022. It is now read-only.

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

tests/get_producers_tests.cpp Outdated Show resolved Hide resolved

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.

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.