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

Remove Copy Bound from NodeId #1250

Closed
drmingdrmer opened this issue Oct 8, 2024 · 2 comments · Fixed by #1255
Closed

Remove Copy Bound from NodeId #1250

drmingdrmer opened this issue Oct 8, 2024 · 2 comments · Fixed by #1255
Assignees

Comments

@drmingdrmer
Copy link
Member

Remove Copy Bound from NodeId

Current Situation

The NodeId type is currently defined in src/node.rs as:

type NodeId: .. + Copy + .. + 'static;

This definition restricts applications from using non-Copy types such as String as NodeId.

Proposed Change

We propose removing the Copy bound from NodeId. This modification will allow the use of non-Copy types as NodeId, providing greater flexibility for applications that prefer variable-length strings or other non-Copy types for node identification.

These change should maintain compatibility and must not break applications.

Implementation Details

To implement this change, we need to:

  1. Replace all copy operations with explicit .clone() calls.
  2. Update derived Copy implementations with manual implementations where necessary, in order to maintain compatibility. For example:
    // Before
    #[derive(Copy...)]
    pub struct LogId<NID: NodeId> {}
    
    // After
    impl<NID: Copy> Copy for LogId<NID> {}

Performance Considerations

Our analysis suggests that the performance impact of this change will be negligible for simple objects that don't involve heap allocation. Benchmarks comparing Clone and Copy for non-heap-allocated objects show nearly identical performance:

Benchmark Results

This means:

  • Existing applications using Copy types for NodeId should see no significant performance changes.
  • New applications using non-Copy types for NodeId may experience a slight performance difference, but it should be minimal in most cases.

Benefits

This change will:

  1. Increase flexibility for NodeId type selection in applications.
  2. Allow the use of more complex or variable-length identifiers when needed.
  3. Maintain performance for existing use cases while opening up new possibilities.

We believe this modification will enhance Openraft's versatility without compromising its current functionality or performance.

Copy link

github-actions bot commented Oct 8, 2024

👋 Thanks for opening this issue!

Get help or engage by:

  • /help : to print help messages.
  • /assignme : to assign this issue to you.

@SteveLauC
Copy link
Collaborator

I second this!

Actually, I was indeed surprised that the NodeId needs to be Copy when accessing Openraft for the first time.

New applications using non-Copy types for NodeId may experience a slight performance difference, but it should be minimal in most cases.

Perhaps we could document the impact of the .clone() within Openraft, something like:

Be aware that NodeId will be .clone()d within Openraft so that heap-allocated NodeId could lead to slightly bad performance.

drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Oct 12, 2024
The `NodeId` type is currently defined as:

```rust
type NodeId: .. + Copy + .. + 'static;
```

This commit removes the `Copy` bound from `NodeId`.
This modification will allow the use of non-`Copy` types as `NodeId`,
providing greater flexibility for applications that prefer
variable-length strings or other non-`Copy` types for node
identification.

This change maintain compatibility by updating derived `Copy`
implementations with manual implementations:

```rust
// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}
```

- Fix: databendlabs#1250
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Oct 13, 2024
The `NodeId` type is currently defined as:

```rust
type NodeId: .. + Copy + .. + 'static;
```

This commit removes the `Copy` bound from `NodeId`.
This modification will allow the use of non-`Copy` types as `NodeId`,
providing greater flexibility for applications that prefer
variable-length strings or other non-`Copy` types for node
identification.

This change maintain compatibility by updating derived `Copy`
implementations with manual implementations:

```rust
// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}
```

- Fix: databendlabs#1250
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Oct 13, 2024
The `NodeId` type is currently defined as:

```rust
type NodeId: .. + Copy + .. + 'static;
```

This commit removes the `Copy` bound from `NodeId`.
This modification will allow the use of non-`Copy` types as `NodeId`,
providing greater flexibility for applications that prefer
variable-length strings or other non-`Copy` types for node
identification.

This change maintain compatibility by updating derived `Copy`
implementations with manual implementations:

```rust
// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}
```

- Fix: databendlabs#1250
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Oct 13, 2024
The `NodeId` type is currently defined as:

```rust
type NodeId: .. + Copy + .. + 'static;
```

This commit removes the `Copy` bound from `NodeId`.
This modification will allow the use of non-`Copy` types as `NodeId`,
providing greater flexibility for applications that prefer
variable-length strings or other non-`Copy` types for node
identification.

This change maintain compatibility by updating derived `Copy`
implementations with manual implementations:

```rust
// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}
```

- Fix: databendlabs#1250
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Oct 13, 2024
The `NodeId` type is currently defined as:

```rust
type NodeId: .. + Copy + .. + 'static;
```

This commit removes the `Copy` bound from `NodeId`.
This modification will allow the use of non-`Copy` types as `NodeId`,
providing greater flexibility for applications that prefer
variable-length strings or other non-`Copy` types for node
identification.

This change maintain compatibility by updating derived `Copy`
implementations with manual implementations:

```rust
// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}
```

- Fix: databendlabs#1250
@drmingdrmer drmingdrmer self-assigned this Oct 13, 2024
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Oct 14, 2024
The `NodeId` type is currently defined as:

```rust
type NodeId: .. + Copy + .. + 'static;
```

This commit removes the `Copy` bound from `NodeId`.
This modification will allow the use of non-`Copy` types as `NodeId`,
providing greater flexibility for applications that prefer
variable-length strings or other non-`Copy` types for node
identification.

This change maintain compatibility by updating derived `Copy`
implementations with manual implementations:

```rust
// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}
```

- Fix: databendlabs#1250
getong pushed a commit to getong/openraft that referenced this issue Oct 15, 2024
The `NodeId` type is currently defined as:

```rust
type NodeId: .. + Copy + .. + 'static;
```

This commit removes the `Copy` bound from `NodeId`.
This modification will allow the use of non-`Copy` types as `NodeId`,
providing greater flexibility for applications that prefer
variable-length strings or other non-`Copy` types for node
identification.

This change maintain compatibility by updating derived `Copy`
implementations with manual implementations:

```rust
// Before
#[derive(Copy...)]
pub struct LogId<NID: NodeId> {}

// After
impl<NID: Copy> Copy for LogId<NID> {}
```

- Fix: databendlabs#1250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants