Skip to content

minicbor-lease adapter for encoding straight into a lease #2164

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

Open
wants to merge 7 commits into
base: eliza/snitch-again
Choose a base branch
from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jul 18, 2025

Based on a suggestion from @mkeeter, this branch adds an adapter that implements minicbor::encode::write::Write for a cursor into a writable lease. This lets us avoid double-buffering in the read_ereports path, where we would previously encode each ereport into the receive buffer, and then copy the contents of that into the lease if there is space for it. The new approach avoids the memcpy, and also doesn't encode the ereport in the case where there isn't space left for it.

I implemented the lease-writer adapter thing in a separate minicbor-lease crate, as I was hoping it would be useful elsewhere...but I'm not actually sure if it will be, now that I think of it: the IPC for delivering an ereport to packrat leases the data from the caller, so callers will just encode into a normal buffer they own. Ah well.

@hawkw hawkw requested review from cbiffle and mkeeter July 18, 2025 19:26
@hawkw
Copy link
Member Author

hawkw commented Jul 18, 2025

@mkeeter Not sure whether you'd prefer to review this as its own PR or if I should just go ahead and merge it straight into #2126 --- whatever you'd prefer works for me!

@mkeeter
Copy link
Collaborator

mkeeter commented Jul 18, 2025

I'd vote to keep it separate! Review to come (either this afternoon or Monday)

@hawkw
Copy link
Member Author

hawkw commented Jul 18, 2025

@mkeeter opened an upstream issue to make the minicbor API less annoying: twittner/minicbor#36

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.

2 participants