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.

Sign up for GitHub

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

Jump to bottom

Remove Copy Bound from NodeId #1250

Open
drmingdrmer opened this issue Oct 8, 2024 · 2 comments · May be fixed by #1255
Open

Remove Copy Bound from NodeId #1250

drmingdrmer opened this issue Oct 8, 2024 · 2 comments · May be fixed by #1255
Assignees
@drmingdrmer
Labels
improvement

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.
@github-actions GitHub Actions
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
@drmingdrmer
Refactor: remvoe Copy bound from NodeId
b0c3dee 
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 linked a pull request Oct 12, 2024 that will close this issue
Open
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Oct 13, 2024
@drmingdrmer
Refactor: remvoe Copy bound from NodeId
a4f9bfb 
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
@drmingdrmer
Refactor: remvoe Copy bound from NodeId
c1e5faf 
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
@drmingdrmer
Refactor: remvoe Copy bound from NodeId
740010d 
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
@drmingdrmer
Refactor: remvoe Copy bound from NodeId
ed3b446 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@drmingdrmer drmingdrmer

Labels
improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor: remvoe Copy bound from NodeId drmingdrmer/openraft
2 participants
@drmingdrmer @SteveLauC