Skip to content

Commit

Permalink
Don't impl PartialEq/Eq for SharedReference and wrapper types
Browse files Browse the repository at this point in the history
  • Loading branch information
bgw committed Jul 23, 2024
1 parent 7677ac8 commit 5b675e5
Show file tree
Hide file tree
Showing 8 changed files with 73 additions and 41 deletions.
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);
}
}

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)))
}

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(())
}
}

0 comments on commit 5b675e5

Please sign in to comment.