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

Replace Sentinel with Self::dangling() for uninitialized EcoVec #28

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

Kmeakin
Copy link
Contributor

@Kmeakin Kmeakin commented Aug 1, 2023

This allows EcoVec::new() to be const.

I believe this is safe. All tests pass with cargo miri test, and I have given my reasoning in the comments. It also does not make EcoVec::as_slice() any more expensive.

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03% 🎉

Comparison is base (9aa2b9e) 91.59% compared to head (46ff467) 91.62%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   91.59%   91.62%   +0.03%     
==========================================
  Files           3        3              
  Lines        1047     1039       -8     
==========================================
- Hits          959      952       -7     
+ Misses         88       87       -1     
Files Changed Coverage Δ
src/vec.rs 91.16% <100.00%> (+0.04%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Kmeakin Kmeakin changed the title Replace Sentinel with NonNull::dangling() for uninitialzed EcoVec Replace Sentinel with NonNull::dangling() for uninitialized EcoVec Aug 1, 2023
@laurmaedje
Copy link
Member

laurmaedje commented Aug 1, 2023

Interesting observation with the header. It's only possible through an optimization that's more recent than the sentinel mechanic (the fact that an EcoVec's pointer points behind its header).

Technically, I don't think there is any guarantee that NonNull<u8>::dangling() is actually 1 though, is there?

I still think the approach has potential, maybe we'd have to drop lower and construct the dangling pointer ourselves to get the guarantees we need. Directly from the value mem::size_of::<Header>().

src/vec.rs Outdated Show resolved Hide resolved
@laurmaedje
Copy link
Member

laurmaedje commented Aug 1, 2023

I think if we use NonNull::new_unchecked(Self::offset() as *mut u8) as a dangling pointer, then we can make things const and eliminate the branch in data() for any type regardless of alignment. It can't ever be the ptr of a valid EcoVec because the pointer is always shifted by the offset, which means the original allocation would have been exactly a null pointer.

It's kind of funny, actually. Vec can't use the null pointer for initialization because of slice deref. We kind of can, except that our null pointer like all our pointers is shifted by the offset, making it non-null. Thanks for bringing this wonderful insight to my attention!

@Kmeakin Kmeakin changed the title Replace Sentinel with NonNull::dangling() for uninitialized EcoVec Replace Sentinel with Self::dangling() for uninitialized EcoVec Aug 1, 2023
@laurmaedje
Copy link
Member

laurmaedje commented Aug 1, 2023

The changes look great! Most of the explanation is already there in the doc comment, but it would be great if there was also an explicit Safety: block before the unsafe block in Self::dangling to explain why it is safe (i.e. why Self::offset() will never be zero). Other than that, it looks good to merge!

Edit: Could you also wrap the doc comment at 80 columns?

@laurmaedje laurmaedje merged commit a4dea19 into typst:main Aug 1, 2023
4 checks passed
@laurmaedje
Copy link
Member

Thanks for the idea and implementation!

@Kmeakin Kmeakin deleted the remove-sentinel branch August 1, 2023 15:34
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