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

Enum Ptr 0.2.0 Plan #1

Closed
7 of 9 tasks
QuarticCat opened this issue Nov 14, 2022 · 11 comments
Closed
7 of 9 tasks

Enum Ptr 0.2.0 Plan #1

QuarticCat opened this issue Nov 14, 2022 · 11 comments

Comments

@QuarticCat
Copy link
Owner

QuarticCat commented Nov 14, 2022

  • Remove unit variant support and named variant support
  • Support generic type params
  • Change Aligned::Pointee to Aligned::ALIGNMENT
  • Support Rc and Arc
  • Rename feature std to alloc
  • Rename map_ref_mut to map_mut
  • Add abilities to use high bits
  • Add more methods to smooth user experience
  • Deal with the new discriminant value grammar
@ryoqun
Copy link
Collaborator

ryoqun commented Sep 22, 2024

@QuarticCat hi, I'm interested in using this crate. for that, Arc support is required. so commenting on this issue.

I wonder releasing 0.2.0 is strictly blocked until all the checkboxes above is marked...

also, as for cautions regarding Rc and Arc as noted in the source comment. are you referring to the fact it isn't necessarily guaranteed that align_of<{RcBox,ArcInner}<T>>() == align_of<T>?

if this is right, how about adding build.rs, which triggers only under nightly, that checks the actually-passed Layouts from Rc/Arc by hooking (nightly-only) custom allocators?

@QuarticCat
Copy link
Owner Author

@ryoqun 0.2.0 is blocked simply because no one is really using it, including myself. So I stopped making progress.

Besides, due to limitations of current type system, this crate is not as handy as I expected. I personally don't like separating Compact and CompactCopy. It's because Compact requires a manually implemented Drop which is not allowed by the Copy trait. I don't like generating borrow structs for users as well but I can't find a better solution.

I'm considering archiving it. If you'd like to take over this repo, I can offer some help.

Are you referring to the fact it isn't necessarily guaranteed that align_of<{RcBox,ArcInner}<T>>() == align_of<T>?

For example, an implementation can use u32 for reference counters. In this case, the alignment will be max(align_of::<T>(), align_of::<u32>()).

How about adding build.rs...

I see no reason to change the layout of RcBox. Making this assumption should be fine in the foreseeable future.

@ryoqun
Copy link
Collaborator

ryoqun commented Sep 24, 2024

@QuarticCat thanks for replaying.

@ryoqun 0.2.0 is blocked simply because no one is really using it, including myself. So I stopped making progress.

in that case, could you publish 0.2.0 as-is if possible?

I personally don't like separating Compact and CompactCopy. It's because Compact requires a manually implemented Drop which is not allowed by the Copy trait.

I see. yeah, seems no easy solution to this. fyi, I'm not going to use CompactCopy.

I don't like generating borrow structs for users as well but I can't find a better solution.

I'll agree with this obstacle and lack of proper solution on this... again, I'm fine with this limitation existing.

I'm considering archiving it. If you'd like to take over this repo, I can offer some help.

Thanks for kind offer! I'm satisfied with the functionalities of 0.2.0 as-is. that said, i can take over maintaining this crate if it's too busy for you to publish the 0.2.0 as-is. fyi, my account on crates.io is https://crates.io/users/ryoqun

Are you referring to the fact it isn't necessarily guaranteed that align_of<{RcBox,ArcInner}<T>>() == align_of<T>?

For example, an implementation can use u32 for reference counters. In this case, the alignment will be max(align_of::<T>(), align_of::<u32>()).

How about adding build.rs...

I see no reason to change the layout of RcBox. Making this assumption should be fine in the foreseeable future.

got it. thanks for explaining.

due to limitations of current type system, this crate is not as handy as I expected.

lastly, as for my use-cases. i fond this crate useful in 2 ways while it's really ergonomic at the same time. (note that the referenced code is messy and in draft...)

  1. Reduce crossbeam's payload from 16 bytes to 8 byte to reduce contention (only .into() is used.): https://github.com/ryoqun/solana/blob/8ba8de5612d4dd295764df63aa85a0e4c94bcc16/unified-scheduler-pool/src/lib.rs#L606
  2. Minimize in-memory index data structure from 16 bytes to 8 bytes to reduce cache miss (.into() and .map_ref() is enough for me in this case): https://github.com/ryoqun/solana/blob/8ba8de5612d4dd295764df63aa85a0e4c94bcc16/unified-scheduler-logic/src/lib.rs#L667

@ryoqun
Copy link
Collaborator

ryoqun commented Sep 29, 2024

@QuarticCat hi, this is a friendly ping. hope my reply finds you well.

@QuarticCat
Copy link
Owner Author

QuarticCat commented Sep 29, 2024

@ryoqun Sorry I missed your last message. I've invited you as repo collaborator and crate owner.

BTW, I just read through the latest code. In a1fb764 I made a try to combine Compact and CompactCopy. It's achieved by hiding the actual type behind a sealed trait to bypass some limitations. Users still need to specify #[enum_ptr(copy)] though. You must decide whether to apply this change before releasing 0.2.0 because it's a breaking change.

@ryoqun
Copy link
Collaborator

ryoqun commented Oct 7, 2024

@ryoqun Sorry I missed your last message. I've invited you as repo collaborator and crate owner.

thanks! I've confirmed both invitations.

BTW, I just read through the latest code. In a1fb764 I made a try to combine Compact and CompactCopy. It's achieved by hiding the actual type behind a sealed trait to bypass some limitations. Users still need to specify #[enum_ptr(copy)] though. You must decide whether to apply this change before releasing 0.2.0 because it's a breaking change.

I reviewed that change. and it looks good to me. I tried hard to lift the requirement of manual annotation of #[enum_ptr(copy)] if T: Copy. but no avail...

Overall, I'm thinking it's okay to publish 0.2.0 as-is with the tip of the main branch. So, preping for it with bunch of pr (ci and version bump): ryoqun#2 and #8

@ryoqun
Copy link
Collaborator

ryoqun commented Oct 8, 2024

@QuarticCat as said yesterday, i think it's ready to publish 0.2.0 after merging this: #8. could you review the pr in your free time? really thanks for timely code reviews so far. Lastly, who should publish to crates.io, me or you?? if it's too bothersome, i can do that now.

fyi, I'm planning to use enum-ptr with yet another crate called rclite. so, I'm planning to create a pr to the downstream crate: ryoqun/rclite#1

@QuarticCat
Copy link
Owner Author

@ryoqun You can publish it.

@ryoqun
Copy link
Collaborator

ryoqun commented Oct 10, 2024

@ryoqun You can publish it.

@QuarticCat I tried. but it looks like i need to be an owner of enum-ptr-derive crate, in addition to enum-ptr as well on the crates.io...

@QuarticCat
Copy link
Owner Author

@ryoqun Invitation has been sent. Sorry for my miss...

@ryoqun
Copy link
Collaborator

ryoqun commented Oct 11, 2024

@ryoqun Invitation has been sent. Sorry for my miss...

no problem. seems i successfully published 0.2.0 thanks!: https://crates.io/crates/enum-ptr/0.2.0

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

No branches or pull requests

2 participants