Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBOR maps use Vec instead of BTreeMap #303

Merged
merged 9 commits into from
Apr 13, 2021

Conversation

kaczmarczyck
Copy link
Collaborator

@kaczmarczyck kaczmarczyck commented Apr 8, 2021

We only access CBOR maps by iterating over them. They also have to be sorted, but that is asserted when reading, and as we control the inputs, we can output the elements in the correct order (we do sort before writing though, to make sure). Therefore, maps can be represented internally by Vec<Key, Value>.

This change intends to reduce the binary size and RAM usage of OpenSK.

This changes removes all occurances of BTreeMap. Binary size in optimized mode is reduced from 130.5 KiB to 126.9 KiB as output by:

echo 'opt-level = "z"' >> Cargo.toml
cargo bloat --release --target=thumbv7em-none-eabi --features=with_ctap1

This PR also cleans up some test code:

  • cbor_array_vec is only used when actually needing to take a vec, not for `cbor_array_vec[vec![...]]
  • arguments are sorted correctly (so that removing the sort_by call in cbor::writer could be removed)

All changes made are (impactful) implementation details, but we don't change any behaviour.

@kaczmarczyck kaczmarczyck self-assigned this Apr 8, 2021
@google-cla google-cla bot added the cla: yes label Apr 8, 2021
@ia0
Copy link
Member

ia0 commented Apr 8, 2021

Could you provide from which number the binary size is reduced from in the description? (both for base reference and history) Ex: Binary size is reduced from XXX.X KiB to 126.9 KiB. Thanks!

Copy link
Collaborator

@jmichelp jmichelp left a comment

Choose a reason for hiding this comment

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

Overall LGTM.
One question: CBOR being canonical, I guess that there's unicity on the keys in a map. Do we have a test for that and isn't an implementation based on Vec<> problematic here (i.e. we would need to sort then check for duplicate keys)?

@kaczmarczyck
Copy link
Collaborator Author

True! Dedup and test added.

The new binary size is 127.3 KiB, and I edited above correctly state the prior size.

Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

LGTM (wasn't planning to review it, it's just that when giving an improvement it's good to have both old and new value to understand the impact)

libraries/cbor/src/macros.rs Outdated Show resolved Hide resolved
libraries/cbor/src/values.rs Outdated Show resolved Hide resolved
libraries/cbor/src/writer.rs Show resolved Hide resolved
libraries/cbor/src/writer.rs Show resolved Hide resolved
src/ctap/mod.rs Show resolved Hide resolved
src/ctap/response.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants