Skip to content

Share Overload signatures behind Arc to avoid deep-copying on clone #3965

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

Open
alexander-beedie wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Share Overload signatures behind Arc to avoid deep-copying on clone #3965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ debug = 1
debug = true
inherits = "dev"

[profile.profiling]
inherits = "release"
debug = 2
strip = "none"

[workspace]
members = [
"crates/pyrefly_build",
Expand Down
2 changes: 1 addition & 1 deletion crates/pyrefly_types/src/callable_residual.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ impl Type {
.cloned()
.map(|branch_ty| branch_ty.into_overload_signature(&metadata))
.collect::<Option<Vec<_>>>()?;
let signatures = Vec1::try_from_vec(signatures).ok()?;
let signatures = Arc::new(Vec1::try_from_vec(signatures).ok()?);
Some(Type::Overload(Overload {
signatures,
metadata: Box::new(metadata),
Expand Down
8 changes: 4 additions & 4 deletions crates/pyrefly_types/src/display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2379,13 +2379,13 @@ pub mod tests {
};

let overload = Type::Overload(Overload {
signatures: vec1![
signatures: Arc::new(vec1![
OverloadType::Function(sig1.clone()),
OverloadType::Forall(Forall {
tparams: fake_tparams(vec![fake_tparam(8, "T", QuantifiedKind::TypeVar)]),
body: sig2.clone()
})
],
]),
metadata: Box::new(sig1.metadata.clone()),
});

Expand Down Expand Up @@ -2421,13 +2421,13 @@ def overloaded_func[T](
let bound_method_overload = Type::BoundMethod(Box::new(BoundMethod {
obj: Type::any_explicit(),
func: BoundMethodType::Overload(Overload {
signatures: vec1![
signatures: Arc::new(vec1![
OverloadType::Function(sig1.clone()),
OverloadType::Forall(Forall {
tparams: fake_tparams(vec![fake_tparam(9, "T", QuantifiedKind::TypeVar)]),
body: sig2
})
],
]),
metadata: Box::new(sig1.metadata),
}),
}));
Expand Down
24 changes: 20 additions & 4 deletions crates/pyrefly_types/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ impl BoundMethodType {
Self::Function(func) => func.signature.subst_self_type_mut(replacement),
Self::Forall(forall) => forall.body.signature.subst_self_type_mut(replacement),
Self::Overload(overload) => {
for sig in overload.signatures.iter_mut() {
for sig in Arc::make_mut(&mut overload.signatures).iter_mut() {
sig.subst_self_type_mut(replacement)
}
}
Expand Down Expand Up @@ -478,10 +478,26 @@ impl BoundMethodType {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[derive(Visit, VisitMut, TypeEq)]
pub struct Overload {
pub signatures: Vec1<OverloadType>,
// Shared behind an Arc so that cloning an Overload is a
// refcount bump instead of a deep copy of every signature.
pub signatures: Arc<Vec1<OverloadType>>,
pub metadata: Box<FuncMetadata>,
}

// Arc<Vec1<OverloadType>> needs explicit Visit/VisitMut impls because there
// is no blanket impl for Arc; VisitMut clones-on-write (matching TArgs).
impl Visit<Type> for Arc<Vec1<OverloadType>> {
fn recurse<'a>(&'a self, f: &mut dyn FnMut(&'a Type)) {
self.as_ref().visit(f);
}
}

impl VisitMut<Type> for Arc<Vec1<OverloadType>> {
fn recurse_mut(&mut self, f: &mut dyn FnMut(&mut Type)) {
Arc::make_mut(self).visit_mut(f);
}
}

impl Overload {
fn is_typeguard(&self) -> bool {
self.signatures.iter().any(|t| t.is_typeguard())
Expand Down Expand Up @@ -1699,7 +1715,7 @@ impl Type {
BoundMethodType::Function(func) => f(&mut func.signature),
BoundMethodType::Forall(forall) => f(&mut forall.body.signature),
BoundMethodType::Overload(overload) => {
for x in overload.signatures.iter_mut() {
for x in Arc::make_mut(&mut overload.signatures).iter_mut() {
match x {
OverloadType::Function(function) => f(&mut function.signature),
OverloadType::Forall(forall) => f(&mut forall.body.signature),
Expand All @@ -1708,7 +1724,7 @@ impl Type {
}
},
Type::Overload(overload) => {
for x in overload.signatures.iter_mut() {
for x in Arc::make_mut(&mut overload.signatures).iter_mut() {
match x {
OverloadType::Function(function) => f(&mut function.signature),
OverloadType::Forall(forall) => f(&mut forall.body.signature),
Expand Down
2 changes: 1 addition & 1 deletion pyrefly/lib/alt/call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
TargetWithTParams(None, *func),
))),
Type::Overload(overload) => {
let funcs = overload.signatures.mapped(|ty| match ty {
let funcs = Arc::unwrap_or_clone(overload.signatures).mapped(|ty| match ty {
OverloadType::Function(function) => TargetWithTParams(None, function),
OverloadType::Forall(forall) => {
TargetWithTParams(Some(forall.tparams), forall.body)
Expand Down
61 changes: 32 additions & 29 deletions pyrefly/lib/alt/class/class_field.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,23 +730,26 @@ impl ClassField {
}
}
Type::Overload(Overload { signatures, .. }) => {
signatures.iter_mut().for_each(|sig| match sig {
OverloadType::Function(body)
if let Some(tparams) = prepend_class_tparams_if_used(body, None) =>
{
*sig = OverloadType::Forall(Forall {
tparams,
body: body.clone(),
})
}
OverloadType::Forall(Forall { tparams, body })
if let Some(new_tparams) =
prepend_class_tparams_if_used(body, Some(tparams)) =>
{
*tparams = new_tparams;
}
_ => {}
});
Arc::make_mut(signatures)
.iter_mut()
.for_each(|sig| match sig {
OverloadType::Function(body)
if let Some(tparams) =
prepend_class_tparams_if_used(body, None) =>
{
*sig = OverloadType::Forall(Forall {
tparams,
body: body.clone(),
})
}
OverloadType::Forall(Forall { tparams, body })
if let Some(new_tparams) =
prepend_class_tparams_if_used(body, Some(tparams)) =>
{
*tparams = new_tparams;
}
_ => {}
});
}
ty => {
if !cls_tparams.is_empty() {
Expand Down Expand Up @@ -2807,15 +2810,16 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
Type::Forall(f) => forall(f.body.clone(), &quantified, Some(&f.tparams))
.map_or(ty, |forall| self.heap.mk_forall(forall)),
Type::Overload(overload) => self.heap.mk_overload(Overload {
signatures: overload.signatures.clone().mapped(|sig| match &sig {
OverloadType::Function(func) => {
forall(func.clone(), &quantified, None).map_or(sig, OverloadType::Forall)
}
OverloadType::Forall(Forall { tparams, body }) => {
forall(body.clone(), &quantified, Some(tparams))
.map_or(sig, OverloadType::Forall)
signatures: Arc::new((*overload.signatures).clone().mapped(|sig| {
match &sig {
OverloadType::Function(func) => forall(func.clone(), &quantified, None)
.map_or(sig, OverloadType::Forall),
OverloadType::Forall(Forall { tparams, body }) => {
forall(body.clone(), &quantified, Some(tparams))
.map_or(sig, OverloadType::Forall)
}
}
}),
})),
metadata: overload.metadata.clone(),
}),
_ => ty,
Expand Down Expand Up @@ -2878,7 +2882,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
&& filtered_sigs.len() < overload.signatures.len()
{
return Some(Overload {
signatures: filtered_sigs,
signatures: Arc::new(filtered_sigs),
metadata: overload.metadata.clone(),
});
}
Expand Down Expand Up @@ -4707,15 +4711,14 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
OverloadType::Function(f) => f.signature.get_first_param(),
OverloadType::Forall(forall) => forall.body.signature.get_first_param(),
};
let applicable: Vec<_> = overload
.signatures
let applicable: Vec<_> = Arc::unwrap_or_clone(overload.signatures)
.into_iter()
.filter(|sig| {
self_param(sig)
.is_none_or(|param| self.is_subset_eq(&child_type, &param))
})
.collect();
let signatures = vec1::Vec1::try_from_vec(applicable).ok()?;
let signatures = Arc::new(vec1::Vec1::try_from_vec(applicable).ok()?);
Some(
BoundMethod {
obj,
Expand Down
12 changes: 6 additions & 6 deletions pyrefly/lib/alt/class/typed_dict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
}

let ty = self.heap.mk_overload(Overload {
signatures: vec1![
signatures: Arc::new(vec1![
OverloadType::Function(Function {
signature: Callable::list(ParamList::new(kw_params), self.heap.mk_none()),
metadata: FuncMetadata::method(cls, dunder::INIT),
Expand All @@ -367,7 +367,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
signature: Callable::list(ParamList::new(map_params), self.heap.mk_none()),
metadata: FuncMetadata::method(cls, dunder::INIT),
})
],
]),
metadata: Box::new(FuncMetadata::method(cls, dunder::INIT)),
});
ClassSynthesizedField::new(ty)
Expand Down Expand Up @@ -458,7 +458,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
metadata: metadata.clone(),
});

let signatures = vec1![partial_overload, tuple_overload, overload_kwargs];
let signatures = Arc::new(vec1![partial_overload, tuple_overload, overload_kwargs]);

ClassSynthesizedField::new(self.heap.mk_overload(Overload {
signatures,
Expand Down Expand Up @@ -616,7 +616,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
value_ty,
));
ClassSynthesizedField::new(self.heap.mk_overload(Overload {
signatures,
signatures: Arc::new(signatures),
metadata: Box::new(metadata),
}))
}
Expand Down Expand Up @@ -707,7 +707,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
&mut literal_signatures,
);
}
let signatures = Vec1::try_from_vec(literal_signatures).ok()?;
let signatures = Arc::new(Vec1::try_from_vec(literal_signatures).ok()?);
Some(ClassSynthesizedField::new(self.heap.mk_overload(
Overload {
signatures,
Expand Down Expand Up @@ -760,7 +760,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
}
Some(ClassSynthesizedField::new(self.heap.mk_overload(
Overload {
signatures: Vec1::try_from_vec(overloads).ok()?,
signatures: Arc::new(Vec1::try_from_vec(overloads).ok()?),
metadata: Box::new(metadata),
},
)))
Expand Down
11 changes: 6 additions & 5 deletions pyrefly/lib/alt/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,10 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
let metadata = self.merge_overload_metadata_no_implementation(&acc);
let func_name = acc.first().2.kind.function_name().into_owned();
Type::Overload(Overload {
signatures: self
.extract_signatures(&func_name, acc, errors)
.mapped(|(_, sig)| sig),
signatures: Arc::new(
self.extract_signatures(&func_name, acc, errors)
.mapped(|(_, sig)| sig),
),
metadata: Box::new(metadata.clone()),
})
}
Expand Down Expand Up @@ -375,7 +376,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
);
self.check_signature_consistency(&sigs, &def, errors);
Type::Overload(Overload {
signatures: sigs.mapped(|(_, sig)| sig),
signatures: Arc::new(sigs.mapped(|(_, sig)| sig)),
metadata: Box::new(metadata),
})
}
Expand Down Expand Up @@ -2435,7 +2436,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
.ok()
.map(|signatures| {
Type::Overload(Overload {
signatures,
signatures: Arc::new(signatures),
metadata: overload.metadata.clone(),
})
}),
Expand Down
7 changes: 2 additions & 5 deletions pyrefly/lib/lsp/wasm/signature_help.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,8 @@ impl Transaction<'_> {
// If the coerced type is an Overload, expand it into multiple signatures
// so signature help displays each overload separately.
if let Type::Overload(overload) = coerced {
let callables: Vec<Type> = overload
.signatures
.into_iter()
.map(|s| s.as_type())
.collect();
let callables: Vec<Type> =
overload.signatures.iter().map(|s| s.as_type()).collect();
CallInfo {
callables,
chosen_overload_index: None,
Expand Down
Loading