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

abi/fastly: correct max value length and use scratch buffer in Dictionary#GetBytes #120

Merged
merged 7 commits into from
Jun 24, 2024

Conversation

cee-dub
Copy link
Collaborator

@cee-dub cee-dub commented Jun 18, 2024

This PR updates work from #117 to handle documented limits for config stores correctly.

In #117 a heuristic was applied to buffers to start small, and increase to a size above the documented limit of "8000 characters." Those characters, however, are UTF-8 encoded, and thus could be comprised of as many as 8000 * 3 bytes. This PR correctly adjusts up to that maximum limit, with an intermediate step for up to 8000 UTF-8 encoded ASCII-range characters, which need 8000 bytes.

It also introduces a scratch space buffer for copies across the host/guest boundary and passes up copies of returned values from the config store.

@cee-dub cee-dub self-assigned this Jun 18, 2024
@cee-dub cee-dub requested review from lpereira and tomchappell June 18, 2024 02:47
@fgsch
Copy link
Member

fgsch commented Jun 18, 2024

Have we confirmed both key and values are utf-8 encoded?
I recall at least keys being limited to a small subset of ascii characters, so utf-8 wouldn't apply.

Nevermind this. I was thinking about the names, not the keys (or values).

@cee-dub cee-dub force-pushed the cw/config-store-size branch 2 times, most recently from b39e673 to 200090c Compare June 20, 2024 21:56
cee-dub added 4 commits June 20, 2024 15:28
Takes advantage of wasm being single-threaded, and the internal/external boundary to reduce allocations to the minimum necessary to copy bytes from the hostcall to the guest.
When the bytes are returned to the caller, they're either copied to a fresh `[]byte` or returned as a `string` (implicit copy).
@cee-dub cee-dub force-pushed the cw/config-store-size branch from 0a80e65 to 30ccd01 Compare June 20, 2024 22:30
@cee-dub cee-dub force-pushed the cw/config-store-size branch from 642eb27 to bdfaf9f Compare June 20, 2024 23:36
@cee-dub cee-dub requested review from lpereira and dgryski and removed request for tomchappell June 21, 2024 21:17
@cee-dub cee-dub changed the title abi/fastly: update Dictionary#GetBytes to account for maximum value length abi/fastly: update Dictionary#GetBytes correct max value length Jun 22, 2024
@cee-dub cee-dub changed the title abi/fastly: update Dictionary#GetBytes correct max value length abi/fastly: correct max value length and use scratch buffer in Dictionary#GetBytes Jun 22, 2024
Copy link
Member

@dgryski dgryski left a comment

Choose a reason for hiding this comment

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

I think this is fine to merge, and since it doesn't change the public API we're safe to roll-back if we're unhappy with it.

@cee-dub cee-dub merged commit a650902 into main Jun 24, 2024
10 checks passed
@cee-dub cee-dub deleted the cw/config-store-size branch June 24, 2024 19:04
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.

4 participants