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

feat: add bytes_vec features #86

Merged
merged 1 commit into from
Jan 16, 2024
Merged

feat: add bytes_vec features #86

merged 1 commit into from
Jan 16, 2024

Conversation

quake
Copy link
Member

@quake quake commented Jan 15, 2024

#75 introduced bytes crate in no_std env, which requires ckb2023 hardfork edition or modification on old contract projects.

This PR add a bytes_vec feature, make it easier to upgrading older projects that want to continue using vec:

- molecule = { version = "0.7.5", default-features = false }
+ molecule = { version = "0.8.0", default-features = false, features = ["bytes_vec"] }

Bound::Unbounded => 0,
};
let end = match range.end_bound() {
Bound::Included(&n) => n + 1,
Copy link
Collaborator

@eval-exec eval-exec Jan 15, 2024

Choose a reason for hiding this comment

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

I think it should be n

use std::ops::Bound::*;
use std::ops::RangeBounds;
fn main() {
    assert_eq!((1..=12).end_bound(), Included(&12));
    assert_eq!((1..13).end_bound(), Excluded(&13));
}
$ cargo run
...
$ echo $?
0

Copy link
Member Author

Choose a reason for hiding this comment

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

this file is just reverted from #75 , I didn't look into the detail, @yangby-cryptape could you help to check?

Copy link

Choose a reason for hiding this comment

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

I think the current code is mostly correct. But there's an edge case, when n is usize::MAX. It might be better to pass the range to self.0[..].index directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I misunderstood. It's correct.

use std::ops::Bound;
use std::ops::RangeBounds;
fn main() {
    let a = vec![0, 1, 2, 3, 4, 5];

    let exclude_2 = slice(a.clone(), 0..2);
    let include_2 = slice(a.clone(), 0..=2);

    let exclude_5 = slice(a.clone(), 0..5);
    let include_5 = slice(a.clone(), 0..=5);

    dbg!(&exclude_2);
    dbg!(&include_2);

    dbg!(&exclude_5);
    dbg!(&include_5);
}

pub fn slice(src: Vec<u8>, range: impl RangeBounds<usize>) ->Vec<u8> {
    let len = src.len();
    let begin = match range.start_bound() {
        Bound::Included(&n) => n,
        Bound::Excluded(&n) => n + 1,
        Bound::Unbounded => 0,
    };
    let end = match range.end_bound() {
        Bound::Included(&n) => n + 1,
        Bound::Excluded(&n) => n,
        Bound::Unbounded => len,
    };
    src[begin..end].to_vec()
}
 cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/rust-playground`
[src/main.rs:21] &exclude_2 = [
    0,
    1,
]
[src/main.rs:22] &include_2 = [
    0,
    1,
    2,
]
[src/main.rs:24] &exclude_5 = [
    0,
    1,
    2,
    3,
    4,
]
[src/main.rs:25] &include_5 = [
    0,
    1,
    2,
    3,
    4,
    5,
]

Copy link

Choose a reason for hiding this comment

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

Use SliceIndex for (Bound, Bound)

let range = (range.start_bound.cloned(), range.end_bound().cloned());
self.0.index(range)

Copy link
Collaborator

@eval-exec eval-exec Jan 15, 2024

Choose a reason for hiding this comment

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

Use SliceIndex for (Bound, Bound)

let range = (range.start_bound.cloned(), range.end_bound().cloned());
self.0.index(range)

But the SliceIndex trait is unsafe.

pub unsafe trait SliceIndex<T: ?Sized>: Sealed {

Copy link

Choose a reason for hiding this comment

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

It's safe to use. It the same as self.0[range]. (Actually, self.0[range] is better.)

@eval-exec eval-exec requested a review from driftluo January 15, 2024 11:51
@xxuejie
Copy link
Contributor

xxuejie commented Jan 15, 2024

Personally, I have my doubts regarding this change. So just want to add my 2 cents here.

There was a time where we had to use Vec<u8> as a container for raw bytes, due to RISC-V was a relative new target back when we started. Nowadays, both the codegen quality of RISC-V target, and our knowledge of the whole toolchain has progressed. As a result, we know enough to get Bytes working in CKB now, with or without the 2nd hardfork.

So to me, we should really advocate a path where the reference counted behavior of Bytes structure is fully celebrated. To me it might not be a good time to keep a path where we are emulating Bytes using plain Vec<u8> with "just-memcpy-everything" semantics. I remembered not too long a go when I tried to optimize a Rust smart contract, and the recurring theme is "just avoid memcpy everywhere".

And it's not like the only path available is to wait for 2nd hardfork, we do have a working path now where we can utilize Bytes right now without needing the hardfork, I personally wasn't sure a second workaround is needed here.

So just to add my vote here: I wasn't sure if this is a good idea.

Copy link
Collaborator

@yangby-cryptape yangby-cryptape left a comment

Choose a reason for hiding this comment

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

Codes are fine, but I don't want to make the decision to merge it, since I saw strongly differing opinions.

I keep neutral.

@quake
Copy link
Member Author

quake commented Jan 16, 2024

If a user wants to upgrade from 0.7 to 0.8 before the 2nd hardfork is activated, he has no choice but to adjust the old project configuration and compilation options. As molecule project maintainer, I think it would be more appropriate to give the user the option to keep the same behavior as in the 0.7 version (which is using vec). We can remove this feature flag after the second hardfork activation, maybe in 0.9 version.

@quake quake merged commit e996a62 into master Jan 16, 2024
6 checks passed
@quake quake deleted the quake/feature-vec branch March 18, 2024 03:12
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.

5 participants