Skip to content

Rc/Arc equality checking could short-circuit on equal pointers if T: Eq #42655

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

Closed
joliss opened this issue Jun 14, 2017 · 6 comments
Closed

Rc/Arc equality checking could short-circuit on equal pointers if T: Eq #42655

joliss opened this issue Jun 14, 2017 · 6 comments
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@joliss
Copy link
Contributor

joliss commented Jun 14, 2017

The PartialEq implementations for Rc and Arc currently always defer to the equality implementations for their inner values (rc.rs:770):

    fn eq(&self, other: &Rc<T>) -> bool {
        **self == **other
    }

However, where T: Eq, we may assume that the inner values obey reflexivity (a == a), and so we could short-circuit the comparison if the pointers are equal. That is, instead of checking **self == **other, we could check self.ptr_eq(other) || **self == **other.

It is conceivable that in rare cases the equality-check on the inner value is so cheap that attempting to short-circuit adds more overhead than it saves. However, my sense is that Rc is usually used on types that are too complex to clone cheaply, and so it stands to reason that in most cases, the equality check on the inner value is expensive. (This is certainly true for the use cases I've encountered.) Checking equality on the pointers furthermore is very cheap, since we're dereferencing them anyway.

I'm cautiously volunteering a pull request if people think that this is a reasonable change. Update: Pull request at #42965.

@joliss joliss changed the title Rc/Arc PartialEq could short-circuit with ptr_eq where T: Eq Rc/Arc PartialEq could short-circuit on equal pointers if T: Eq Jun 14, 2017
@joliss joliss changed the title Rc/Arc PartialEq could short-circuit on equal pointers if T: Eq Rc/Arc equality checking could short-circuit on equal pointers if T: Eq Jun 14, 2017
@steveklabnik steveklabnik added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 14, 2017
joliss added a commit to sinopiaolive/package-manager that referenced this issue Jun 19, 2017
We would ideally like to do without this flag and simply check
`new_constraint_set != old_constraint_set`. However, this performs a full (slow)
equality check, because immutable-rs doesn't short-circuit equality checking,
which in turn is because Rc doesn't.
rust-lang/rust#42655
@joliss
Copy link
Contributor Author

joliss commented Jun 26, 2017

Hm, I believe this is actually impossible to implement until we have support for specialization (#31844). Please correct me if I'm wrong.

@sfackler
Copy link
Member

Specialization already exists - it's just unstable. That doesn't matter for use in the standard library, though.

@joliss
Copy link
Contributor Author

joliss commented Jun 29, 2017

Cool, thanks @sfackler. I posted a pull request at #42965.

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 27, 2017
@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 15, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 15, 2017

Tagging C-feature-accepted based on the discussion in #42965. Someone will need to rebase the PR and address the review comments.

@mbrubeck
Copy link
Contributor

mbrubeck commented Mar 6, 2018

This optimization was implemented by servo/servo#18793 in a fork of Arc used in Servo and Firefox.

It occurs to me that if this is valid for Rc<T> and Arc<T> then it should also be valid for &T.

@jdm
Copy link
Contributor

jdm commented Mar 6, 2018

The &T case was discussed in #42965.

@bors bors closed this as completed in 2a916a6 Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants