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

Support zero-copy in UnicodeSet #855

Closed
sffc opened this issue Jul 13, 2021 · 2 comments · Fixed by #922
Closed

Support zero-copy in UnicodeSet #855

sffc opened this issue Jul 13, 2021 · 2 comments · Fixed by #922
Assignees
Labels
blocked A dependency must be resolved before this is actionable C-pluralrules Component: Plural rules S-medium Size: Less than a week (larger bug fix or enhancement) T-core Type: Required functionality

Comments

@sffc
Copy link
Member

sffc commented Jul 13, 2021

We should figure out the right way to make UnicodeSet support zero-copy deserialization. I propose that we change UnicodeSet itself to wrap a ZeroVec, thereby gaining a lifetime parameter.

@sffc sffc added T-core Type: Required functionality C-pluralrules Component: Plural rules discuss-priority Discuss at the next ICU4X meeting labels Jul 13, 2021
@sffc sffc added this to the ICU4X 0.4 milestone Jul 13, 2021
@sffc
Copy link
Member Author

sffc commented Jul 15, 2021

Discussion:

  • @iainireland - How does the lifetime parameter work over FFI?
  • @Manishearth - In JavaScript we can do clever things involving the GC; in C++ you have to be careful. And you can always set the lifetime to 'static.
  • @sffc - What is the return value of Unicode property getters like get_alnum_property?
  • @Manishearth - Could it have 's? Or maybe a lifetime from the DataProvider?
  • @sffc - It could return a DataPayload.
  • @iainireland - As we work on the return type, does it affect what shows up on the C++ side of FFI?
  • @Manishearth - Instead of boxing a UnicodeSet, it would box a DataPayload. So on the C++ side, you wouldn't need to care, but the FFI layer would need to be updated.
  • @iainireland - Does this affect UnicodeSetBuilder or only UnicodeSet?
  • @sffc - Only UnicodeSet.

Conclusion:

  • UnicodeSet should wrap ZeroVec
  • The Unicode property getters should return DataPayload

@sffc sffc added S-medium Size: Less than a week (larger bug fix or enhancement) and removed discuss-priority Discuss at the next ICU4X meeting labels Jul 15, 2021
@echeran
Copy link
Contributor

echeran commented Aug 11, 2021

In the process of applying feedback changes, I made a minimal repro case for what seems to be a compiler bug (filed at: rust-lang/rust#87932)

@echeran echeran added the blocked A dependency must be resolved before this is actionable label Aug 11, 2021
@sffc sffc modified the milestones: ICU4X 0.4, 2021 Q3 0.4 Sprint B Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked A dependency must be resolved before this is actionable C-pluralrules Component: Plural rules S-medium Size: Less than a week (larger bug fix or enhancement) T-core Type: Required functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants