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

Don't impl PartialEq/Eq for SharedReference and wrapper types #8815

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/turbo-tasks-memory/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ impl Cell {
content: ref mut cell_content,
dependent_tasks,
} => {
if content != *cell_content {
if content.ptr_eq(cell_content) {
if !dependent_tasks.is_empty() {
turbo_tasks.schedule_notify_tasks_set(dependent_tasks);
dependent_tasks.clear();
Expand Down
10 changes: 9 additions & 1 deletion crates/turbo-tasks/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ pub struct TaskExecutionSpec<'a> {

// TODO technically CellContent is already indexed by the ValueTypeId, so we
// don't need to store it here
#[derive(Clone, Debug, Default, PartialEq, Eq, Hash, Serialize, Deserialize)]
#[derive(Clone, Debug, Default, Serialize, Deserialize)]
pub struct CellContent(pub Option<SharedReference>);

impl Display for CellContent {
Expand Down Expand Up @@ -377,6 +377,14 @@ impl CellContent {
pub fn try_cast<T: Any + VcValueType>(self) -> Option<ReadRef<T>> {
Some(ReadRef::new_arc(self.0?.downcast().ok()?))
}

pub fn ptr_eq(&self, other: &Self) -> bool {
match (&self.0, &other.0) {
(Some(this), Some(other)) => this.ptr_eq(other),
(None, None) => true,
(_, _) => false,
}
}
}

pub type TaskCollectiblesMap = AutoMap<RawVc, i32, BuildHasherDefault<FxHasher>, 1>;
Expand Down
2 changes: 1 addition & 1 deletion crates/turbo-tasks/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1575,7 +1575,7 @@ impl CurrentCellRef {
let tt = turbo_tasks();
let content = tt.read_own_task_cell(self.current_task, self.index).ok();
let update = if let Some(CellContent(Some(content))) = content {
content != shared_ref
content.ptr_eq(&shared_ref)
} else {
true
};
Expand Down
14 changes: 1 addition & 13 deletions crates/turbo-tasks/src/task/shared_reference.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use std::{
any::Any,
fmt::{Debug, Display},
hash::Hash,
};

use anyhow::Result;
Expand Down Expand Up @@ -32,22 +31,11 @@ impl SharedReference {
Err(data) => Err(Self(self.0, data)),
}
}
}

impl Hash for SharedReference {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
Hash::hash(&(&*self.1 as *const (dyn Any + Send + Sync)), state)
}
}
impl PartialEq for SharedReference {
// Must compare with PartialEq rather than std::ptr::addr_eq since the latter
// only compares their addresses.
#[allow(ambiguous_wide_pointer_comparisons)]
fn eq(&self, other: &Self) -> bool {
pub fn ptr_eq(&self, other: &Self) -> bool {
triomphe::Arc::ptr_eq(&self.1, &other.1)
}
}
impl Eq for SharedReference {}

impl Debug for SharedReference {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
Expand Down
18 changes: 4 additions & 14 deletions crates/turbo-tasks/src/trait_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,20 +40,6 @@ impl<T> Clone for TraitRef<T> {
}
}

impl<T> PartialEq for TraitRef<T> {
fn eq(&self, other: &Self) -> bool {
self.shared_reference == other.shared_reference
}
}

impl<T> Eq for TraitRef<T> {}

impl<T> std::hash::Hash for TraitRef<T> {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.shared_reference.hash(state)
}
}

impl<T> Serialize for TraitRef<T> {
fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
self.shared_reference.serialize(serializer)
Expand Down Expand Up @@ -89,6 +75,10 @@ where
_t: PhantomData,
}
}

pub fn ptr_eq(&self, other: &Self) -> bool {
self.shared_reference.ptr_eq(&other.shared_reference)
}
}

impl<T> TraitRef<T>
Expand Down
12 changes: 6 additions & 6 deletions crates/turbo-tasks/src/value.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{fmt::Debug, marker::PhantomData, ops::Deref};
use std::{fmt::Debug, hash::Hash, marker::PhantomData, ops::Deref};

use serde::{Deserialize, Serialize};

Expand Down Expand Up @@ -91,17 +91,17 @@ impl<T> Clone for TransientInstance<T> {
}
}

impl<T> Eq for TransientInstance<T> {}

impl<T> PartialEq for TransientInstance<T> {
fn eq(&self, other: &Self) -> bool {
self.inner == other.inner
self.inner.ptr_eq(&other.inner)
}
}

impl<T> std::hash::Hash for TransientInstance<T> {
impl<T> Eq for TransientInstance<T> {}

impl<T> Hash for TransientInstance<T> {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.inner.hash(state);
self.inner.1.as_ptr().hash(state);
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe implement ptr_hash on SharedReference

}

Expand Down
25 changes: 24 additions & 1 deletion crates/turbopack-browser/src/ecmascript/list/version.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::hash::Hash;

use anyhow::Result;
use indexmap::IndexMap;
use turbo_tasks::{RcStr, TraitRef, TryJoinIterExt, Vc};
Expand All @@ -9,7 +11,7 @@ type VersionTraitRef = TraitRef<Box<dyn Version>>;
/// The version of a [`EcmascriptDevChunkListContent`].
///
/// [`EcmascriptDevChunkListContent`]: super::content::EcmascriptDevChunkListContent
#[turbo_tasks::value(shared)]
#[turbo_tasks::value(shared, eq = "manual")]
pub(super) struct EcmascriptDevChunkListVersion {
/// A map from chunk path to its version.
#[turbo_tasks(trace_ignore)]
Expand All @@ -19,6 +21,27 @@ pub(super) struct EcmascriptDevChunkListVersion {
pub by_merger: IndexMap<Vc<Box<dyn VersionedContentMerger>>, VersionTraitRef>,
}

fn eq_index_map<K: Hash + Eq, V>(
this: &IndexMap<K, TraitRef<V>>,
other: &IndexMap<K, TraitRef<V>>,
) -> bool {
if this.len() != other.len() {
return false;
}

this.iter()
.all(|(key, value)| other.get(key).map_or(false, |v| value.ptr_eq(v)))
Comment on lines +32 to +33
Copy link
Member

Choose a reason for hiding this comment

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

Order should matter here. You can use zip() instead. That's also more performant.

}

impl PartialEq for EcmascriptDevChunkListVersion {
fn eq(&self, other: &Self) -> bool {
eq_index_map(&self.by_path, &other.by_path)
|| eq_index_map(&self.by_merger, &other.by_merger)
}
}

impl Eq for EcmascriptDevChunkListVersion {}

#[turbo_tasks::value_impl]
impl Version for EcmascriptDevChunkListVersion {
#[turbo_tasks::function]
Expand Down
31 changes: 27 additions & 4 deletions crates/turbopack-core/src/version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub trait VersionedContent {
let to_ref = to.into_trait_ref().await?;

// Fast path: versions are the same.
if from_ref == to_ref {
if from_ref.ptr_eq(&to_ref) {
return Ok(Update::None.into());
}

Expand Down Expand Up @@ -181,15 +181,23 @@ pub enum Update {
}

/// A total update to a versioned object.
#[derive(PartialEq, Eq, Debug, Clone, TraceRawVcs, ValueDebugFormat, Serialize, Deserialize)]
#[derive(Debug, Clone, TraceRawVcs, ValueDebugFormat, Serialize, Deserialize)]
pub struct TotalUpdate {
/// The version this update will bring the object to.
#[turbo_tasks(trace_ignore)]
pub to: TraitRef<Box<dyn Version>>,
}

impl PartialEq for TotalUpdate {
fn eq(&self, other: &Self) -> bool {
self.to.ptr_eq(&other.to)
}
}

impl Eq for TotalUpdate {}

/// A partial update to a versioned object.
#[derive(PartialEq, Eq, Debug, Clone, TraceRawVcs, ValueDebugFormat, Serialize, Deserialize)]
#[derive(Debug, Clone, TraceRawVcs, ValueDebugFormat, Serialize, Deserialize)]
pub struct PartialUpdate {
/// The version this update will bring the object to.
#[turbo_tasks(trace_ignore)]
Expand All @@ -200,6 +208,14 @@ pub struct PartialUpdate {
pub instruction: Arc<serde_json::Value>,
}

impl PartialEq for PartialUpdate {
fn eq(&self, other: &Self) -> bool {
self.to.ptr_eq(&other.to) && self.instruction == other.instruction
}
}

impl Eq for PartialUpdate {}

/// [`Version`] implementation that hashes a file at a given path and returns
/// the hex encoded hash as a version identifier.
#[turbo_tasks::value]
Expand Down Expand Up @@ -260,7 +276,14 @@ impl VersionState {

pub async fn set(self: Vc<Self>, new_version: TraitRef<Box<dyn Version>>) -> Result<()> {
let this = self.await?;
this.version.set(new_version);
this.version.update_conditionally(|version| {
if !version.ptr_eq(&new_version) {
*version = new_version;
true
} else {
false
}
});
Ok(())
}
}
Loading