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

Add support for listing configs #538

Merged
merged 1 commit into from
Oct 17, 2024
Merged

Add support for listing configs #538

merged 1 commit into from
Oct 17, 2024

Conversation

sosthene-nitrokey
Copy link
Collaborator

@sosthene-nitrokey sosthene-nitrokey commented Oct 3, 2024

@sosthene-nitrokey sosthene-nitrokey force-pushed the list-configs branch 2 times, most recently from d13419b to 169f491 Compare October 3, 2024 14:10
@nitrokey-ci
Copy link
Collaborator

nitrokey-ci commented Oct 3, 2024

No significant changes.

Insignifcant changes
metric value change
binary-size-nk3am 1,441,021 🔴 +1,260 (+0.09%)
binary-size-nk3am-test 2,049,743 🔴 +1,890 (+0.09%)
binary-size-nk3xn 516,884 🔴 +576 (+0.11%)
binary-size-nk3xn-test 591,080 🔴 +576 (+0.10%)
binary-size-nkpk 747,970 🔴 +1,575 (+0.21%)

@robin-nitrokey
Copy link
Member

I wonder where the 6k binary size is coming from. My first guess was serialization, and we could offload that to a macro or build script, but a quick test replacing the fields with an empty slice only yields less than 500 bytes. Very confusing.

@sosthene-nitrokey
Copy link
Collaborator Author

sosthene-nitrokey commented Oct 4, 2024

My guess is the added serialization to a heapless::Vec<u8, N> in cbor-smol, which wasn't there in the past? Since the constant and the serialization are defined in completely different crates it could be possible that the constant is not inlined/optimized out?

@robin-nitrokey
Copy link
Member

Ah, by using upstream cbor-smol instead of our fork you basically reverted Nitrokey/cbor-smol#3 and Nitrokey/cbor-smol#4.

@sosthene-nitrokey
Copy link
Collaborator Author

Normally I added both back. The patch points to trussed-dev/cbor-smol#14 and trussed-dev/cbor-smol#8 implements #3

@sosthene-nitrokey
Copy link
Collaborator Author

It didn't fit without.

@robin-nitrokey
Copy link
Member

robin-nitrokey commented Oct 4, 2024

Oh, sorry, I must have ended up on the wrong branch. Anyway, just applying the cbor-smol patch on top of main causes a similar size increase as this PR. Maybe it’s trussed-dev/cbor-smol#8? The heapless change shouldn’t have any effects until it is used, no? I’ll prepare PRs to merge the size optimizations upstream so that we can release 0.4.1 and have a clean baseline.

@sosthene-nitrokey
Copy link
Collaborator Author

The heapless change is used in admin app (because ctaphid-dispatch uses Vec<u8, N> and not Bytes for the response buffer.

@sosthene-nitrokey sosthene-nitrokey force-pushed the list-configs branch 3 times, most recently from 21b53a3 to ed7860e Compare October 7, 2024 08:50
@sosthene-nitrokey
Copy link
Collaborator Author

I think I found a way to improve that by making the heapless-bytes implementation fall back to the heapless implementation.

@robin-nitrokey
Copy link
Member

Starting from the current main (fca2d32), I get these numbers for NK3xN with test:

Conclusions:

  1. In the original patch Never inline map value deserialization trussed-dev/cbor-smol#14, you only included Never inline map value deserialization cbor-smol#1 (map value deserialization) but not Never inline map and tuple deserialization cbor-smol#4 (map and tuple deserialization). This probably caused the 6k increase.
  2. Updating to upstream still causes a 2k increase – PR 4 actually reverted PR 1 which I missed when re-applying the patches. Indeed, after reverting the change, we are back at the original size (or even slightly smaller).
  3. The size optimization in Add support for multiple heapless and heapless-bytes versions trussed-dev/cbor-smol#13 has an adverse effect.

Copy link
Member

@robin-nitrokey robin-nitrokey left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! It looks like the CI is confused because #539 includes the same commit – to be investigated.

Cargo.toml Outdated
fido-authenticator = { git = "https://github.com/Nitrokey/fido-authenticator.git", tag = "v0.1.1-nitrokey.20" }
lpc55-hal = { git = "https://github.com/Nitrokey/lpc55-hal", tag = "v0.3.0-nitrokey.2" }
Copy link
Member

Choose a reason for hiding this comment

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

unused patch

@@ -1118,7 +1139,7 @@ mod tests {
},
fs_version: 1,
};
let data: Bytes<1024> = cbor_serialize_bytes(&config).unwrap();
let data: heapless_bytes::Bytes<1024> = cbor_serialize_bytes(&config).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

We could directly replace this with a cbor_serialize into an array so that we don’t have to update it in the future (and we don’t need to add the explicit heapless_bytes dependency).

@robin-nitrokey robin-nitrokey force-pushed the list-configs branch 2 times, most recently from 68a5cbc to 2c4be6a Compare October 17, 2024 09:42
@robin-nitrokey robin-nitrokey merged commit 76591a5 into main Oct 17, 2024
9 checks passed
@robin-nitrokey robin-nitrokey deleted the list-configs branch October 17, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants