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 borrowed-only versions of data structs in ZeroVec #676

Closed
sffc opened this issue Apr 22, 2021 · 9 comments · Fixed by #1238
Closed

Add borrowed-only versions of data structs in ZeroVec #676

sffc opened this issue Apr 22, 2021 · 9 comments · Fixed by #1238
Assignees
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement) T-enhancement Type: Nice-to-have but not required

Comments

@sffc
Copy link
Member

sffc commented Apr 22, 2021

Part of #1082

Depends on #1078

zerovec is capable of supporting no_std, including no heap allocations. To achieve this, we should make public versions of ZeroVec, VarZeroVec, and ZeroMap that are borrow-only.

Some caveats:

  • Can we make ICU4X data structs no_std at the same time?
  • How to avoid code duplication?

CC @Manishearth

@sffc sffc added T-enhancement Type: Nice-to-have but not required help wanted Issue needs an assignee backlog C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement) labels Apr 22, 2021
@sffc sffc changed the title Add no_std support to zerovec Add borrowed-only versions of data structs in ZeroVec Sep 17, 2021
@sffc sffc added this to the 2021 Q3 0.4 Sprint C milestone Sep 17, 2021
@sffc sffc removed help wanted Issue needs an assignee backlog labels Sep 17, 2021
@Manishearth
Copy link
Member

Manishearth commented Sep 20, 2021

Work items involved here, as an inverted dependency tree:

old list

list moved to #1082

@sffc
Copy link
Member Author

sffc commented Sep 21, 2021

Writing down some of my thoughts:

I feel that the AsVarULE trait is fraught. I think a lot of operations would be easier to reason about if VarZeroVec were parameterized by the VarULE type directly, rather than the AsVarULE type.

For example:

  • VarZeroVec<str> is a vector of strings.
  • VarZeroVec<[u8]> is a vector of blobs.
  • VarZeroVec<FooULE> is a vector of FooULEs, which must be a VarULE type.

Nesting:

  • VarZeroVec<ZeroVecULE<u16>> is a vector of u16 vectors.
  • VarZeroVec<VarZeroVecULE<ZeroVecULE<u16>>> is a 3D vector of u16.

The contract for VarZeroVec is simply that the type in the vector must be #[repr] with a variable-length byte slice.

The other advantage is that by making the VarULE type front and center, we encourage developers to think in terms of the VarULE throughout their program, rather than in some other abstraction that may be less efficient or require conversions to/from the VarULE.

@Manishearth
Copy link
Member

Manishearth commented Sep 21, 2021

Some things to make requirements of the VarULE and ULE trait

  • no padding bytes, all bytes are initialized (allows us to treat them as u8 slices safely)
  • equality is byte equality (should AsVarULE require Eq?)

@sffc
Copy link
Member Author

sffc commented Sep 21, 2021

  • no padding bytes, all bytes are initialized (allows us to treat them as u8 slices safely)

+1

  • equality is byte equality (should AsVarULE require Eq?)

Yes, and we should also require that the parser reject any input that is not in canonical form. For example, "1.00e3" and "10.0e2" are semantically equal, but only the former is in canonical form.

With these new constraints, can people implement ULE/VarULE without any unsafe code? (Maybe except for a single "unsafe" on the trait impl to assert that all the constraints are satisfied.)

@Manishearth
Copy link
Member

Manishearth commented Sep 21, 2021

With these new constraints, can people implement ULE/VarULE without any unsafe code? (Maybe except for a single "unsafe" on the trait impl to assert that all the constraints are satisfied.)

No because they both expose unsafe fns as well. I don't think this is a useful metric to optimize for.

@Manishearth
Copy link
Member

Another potential improvement might be to get rid of AsVarULE entirely and have people directly use ULE types

@sffc
Copy link
Member Author

sffc commented Sep 22, 2021

Related: type structure for ZeroMap. Removing AsVarULE requires making changes to the ZeroMap traits. (Follow-up to #1057 (comment))

@Manishearth Manishearth added the blocked A dependency must be resolved before this is actionable label Sep 22, 2021
@Manishearth
Copy link
Member

I moved the list of work items to #1082. "Get rid of AsVarULE" is #615

@Manishearth
Copy link
Member

As a part of this we should make make_mut() public on ZeroVec (and perhaps rename both to to_mut())

@Manishearth Manishearth added this to the 2021 Q4 0.5 Sprint A milestone Oct 19, 2021
@Manishearth Manishearth removed the blocked A dependency must be resolved before this is actionable label Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-medium Size: Less than a week (larger bug fix or enhancement) T-enhancement Type: Nice-to-have but not required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants