Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

is_unique for ArcArray #1399

Merged
merged 4 commits into from
Jul 29, 2024
Merged

is_unique for ArcArray #1399

merged 4 commits into from
Jul 29, 2024

Conversation

daniellga
Copy link
Contributor

Implement this function to know if the inner Arc is shared.

D: Dimension,
{
/// Returns `true` iff the inner `Arc` is not shared.
pub fn is_unique(&mut self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to make this a non-mut method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't it risk creating a race condition? I kinda got confused on this point... The alternative would be just to sum strong and weak counts:

fn is_unique(&self) -> bool {
    Arc::strong_count(&self.data.0) + Arc::weak_count(&self.data.0) == 1
}

Copy link
Member

@bluss bluss Jul 21, 2024

Choose a reason for hiding this comment

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

I was thinking there are race conditions anyway, because the method only returns a boolean. There's no solving that without creating something entirely different. But there's the &mut itself that maintains some exclusivity, that's true.

What's the use case for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my case I am using an ArcArray as the core of a struct that I call from other language. I want the user of that language to be able to know when a certain method, lets say for example fft calculation, has created a copy of that inner ArcArray.

Copy link
Member

@bluss bluss Jul 21, 2024

Choose a reason for hiding this comment

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

It is confusing, and if there would be weak refs you're right that it's racy. ArcArray doesn't support weak refs at all. That means there are two options - (1) the way of just checking strong count and deciding we can never support Weak refs or (2) your get_mut implementation which is more sure to always be safe.

Any "is_unique -> bool" method is racy unless we can control the current Arc, i.e we are sure nobody's cloning the exact Arc handle we are holding. For this reason, ownership or &mut of the current Arc is needed to be sure of that.

I would say both (1) in combination with holding or having the option to hold a &mut Arc would be just as safe as (2) for ArcArray, but which choice to use doesn't matter so much to me.

If there is code that first .clone()s and then mutate (unsharing) your ArcArray, you won't notice this way, because the final effect is two independent copies of the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are saying something like this for (1)?

fn is_unique(&mut self) -> bool {
    Arc::strong_count(&self.0) == 1
}

I am ok with both choices, I just used the second one because of extensibility, in case you choose to use weak references in the future, for whatever reason. Let me know if you have any better suggestion than this.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's clear it's possible to make it a non-mut method - using strong_count.
That seems to be compatible with the use case as well (your use case - if you want to ensure the Arc is not concurrently cloned, you can hold a &mut to the ArcArray, the method doesn't need to use &mut for that). I can't imagine too many other use cases, other than some user wanting to do something they shouldn't w.r.t mutability and sharing 😆.

Any other ideas about use cases?
Maybe get_mut is the safest choice for unknown use cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will modify it then to count strong count and accept self. Another approach would be to allow access (a getter) to the inner arc, then the user could do whatever he wanted with it.
Just for curiosity, why do you want to make it non-mut?

Copy link
Member

Choose a reason for hiding this comment

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

For non-mut, just the instinct that it's an interrogating method that doesn't modify.
Inner fields won't be exposed 🙂

@bluss
Copy link
Member

bluss commented Jul 28, 2024

Can now be rebased on master because clippy errors are fixed there.

@bluss bluss merged commit e86ebff into rust-ndarray:master Jul 29, 2024
10 checks passed
@bluss
Copy link
Member

bluss commented Jul 29, 2024

Thanks

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants