Skip to content

Commit e8fdb7a

Browse files
samwgoldmanmeta-codesync[bot]
authored andcommitted
Remove fields from ClassInner
Summary: Remove the `fields` field from `ClassInner` and the `fields` parameter from `Class::new()`. Remove all delegating field accessor methods from `Class` (fields, contains, is_field_annotated, is_field_initialized_on_class, field_decl_range, field_docstring_range, class_body_fields). All callers now use `get_class_fields()` via AnswersSolver (for solver code paths) or `Bindings::get_class_fields()` / `Transaction::get_class_fields()` (for external callers like LSP, Pysa, Query). Also migrates get_class_field_map in classdef.rs and the test_fields test in enums.rs. Performance (median of 3 opt-clang-thinlto runs, baseline=parent of stack): PyTorch: | metric | baseline | this diff | cumulative | |--------|----------|-----------|------------| | cpu | 40.65s | 40.83s | +0.4% | | wall | 2.24s | 2.22s | -0.9% | | maxrss | 0.93GB | 0.90GB | -3.2% | Reviewed By: yangdanny97 Differential Revision: D96515736 fbshipit-source-id: 867ef12348e8ec69f5d63acf021321cf323b00c2
1 parent 8733cbf commit e8fdb7a

File tree

6 files changed

+19
-55
lines changed

6 files changed

+19
-55
lines changed

crates/pyrefly_types/src/class.rs

Lines changed: 7 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,13 @@ impl ClassFields {
190190
}
191191
}
192192

193+
/// Align to a cache line to prevent false sharing. `ClassInner` is stored
194+
/// behind `Arc` and accessed concurrently from multiple threads during
195+
/// type checking. Without alignment, multiple `Arc<ClassInner>` allocations
196+
/// can land on the same cache line, causing cross-thread invalidation
197+
/// traffic when any of them are accessed (even read-only, due to the Arc
198+
/// refcount on adjacent allocations).
199+
#[repr(align(64))]
193200
struct ClassInner {
194201
def_index: ClassDefIndex,
195202
qname: QName,
@@ -198,7 +205,6 @@ struct ClassInner {
198205
/// when computing the class tparams). Whenever it is `None`, there will be a corresponding
199206
/// `KeyTParams` / `BindingTParams` pair to compute the class tparams.
200207
precomputed_tparams: Option<Arc<TParams>>,
201-
fields: ClassFields,
202208
}
203209

204210
impl Debug for ClassInner {
@@ -269,20 +275,14 @@ impl Class {
269275
parent: NestingContext,
270276
module: Module,
271277
precomputed_tparams: Option<Arc<TParams>>,
272-
fields: ClassFields,
273278
) -> Self {
274279
Self(Arc::new(ClassInner {
275280
def_index,
276281
qname: QName::new(name, parent, module),
277282
precomputed_tparams,
278-
fields,
279283
}))
280284
}
281285

282-
pub fn contains(&self, name: &Name) -> bool {
283-
self.0.fields.contains(name)
284-
}
285-
286286
pub fn range(&self) -> TextRange {
287287
self.0.qname.range()
288288
}
@@ -319,30 +319,6 @@ impl Class {
319319
self.0.qname.module()
320320
}
321321

322-
pub fn fields(&self) -> impl ExactSizeIterator<Item = &Name> {
323-
self.0.fields.fields()
324-
}
325-
326-
pub fn class_body_fields(&self) -> impl Iterator<Item = &Name> {
327-
self.0.fields.class_body_fields()
328-
}
329-
330-
pub fn is_field_annotated(&self, name: &Name) -> bool {
331-
self.0.fields.is_field_annotated(name)
332-
}
333-
334-
pub fn is_field_initialized_on_class(&self, name: &Name) -> bool {
335-
self.0.fields.is_field_initialized_on_class(name)
336-
}
337-
338-
pub fn field_decl_range(&self, name: &Name) -> Option<TextRange> {
339-
self.0.fields.field_decl_range(name)
340-
}
341-
342-
pub fn field_docstring_range(&self, name: &Name) -> Option<TextRange> {
343-
self.0.fields.field_docstring_range(name)
344-
}
345-
346322
pub fn has_qname(&self, module: &str, parent: &NestingContext, name: &str) -> bool {
347323
self.0.qname.module_name().as_str() == module
348324
&& self.0.qname.parent() == parent

crates/pyrefly_types/src/display.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1128,7 +1128,6 @@ pub mod tests {
11281128
use crate::callable::Required;
11291129
use crate::class::Class;
11301130
use crate::class::ClassDefIndex;
1131-
use crate::class::ClassFields;
11321131
use crate::class::ClassType;
11331132
use crate::heap::TypeHeap;
11341133
use crate::literal::Lit;
@@ -1162,7 +1161,6 @@ pub mod tests {
11621161
NestingContext::toplevel(),
11631162
mi,
11641163
None,
1165-
ClassFields::default(),
11661164
)
11671165
}
11681166

crates/pyrefly_types/src/type_output.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,6 @@ mod tests {
192192
use super::*;
193193
use crate::class::Class;
194194
use crate::class::ClassDefIndex;
195-
use crate::class::ClassFields;
196195
use crate::class::ClassType;
197196
use crate::literal::LitEnum;
198197
use crate::literal::LitStyle;
@@ -217,7 +216,6 @@ mod tests {
217216
NestingContext::toplevel(),
218217
mi,
219218
None,
220-
ClassFields::default(),
221219
)
222220
}
223221

pyrefly/lib/alt/class/classdef.rs

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -72,19 +72,12 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
7272
} else {
7373
Some(self.calculate_class_tparams_no_legacy(name, x.type_params.as_deref(), errors))
7474
};
75-
let fields = self
76-
.bindings()
77-
.metadata()
78-
.get_class(def_index)
79-
.fields
80-
.clone();
8175
Class::new(
8276
def_index,
8377
x.name.clone(),
8478
parent.dupe(),
8579
self.module().dupe(),
8680
precomputed_tparams,
87-
fields,
8881
)
8982
}
9083

@@ -94,19 +87,12 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
9487
name: &Identifier,
9588
parent: &NestingContext,
9689
) -> Class {
97-
let fields = self
98-
.bindings()
99-
.metadata()
100-
.get_class(def_index)
101-
.fields
102-
.clone();
10390
Class::new(
10491
def_index,
10592
name.clone(),
10693
parent.dupe(),
10794
self.module().dupe(),
10895
Some(Arc::new(TParams::default())),
109-
fields,
11096
)
11197
}
11298

@@ -131,10 +117,12 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
131117
}
132118

133119
pub fn get_class_field_map(&self, cls: &Class) -> SmallMap<Name, Arc<ClassField>> {
134-
let fields = cls.fields();
135-
let mut map = SmallMap::with_capacity(fields.len());
120+
let Some(class_fields) = self.get_class_fields(cls) else {
121+
return SmallMap::new();
122+
};
123+
let mut map = SmallMap::with_capacity(class_fields.len());
136124

137-
for name in fields {
125+
for name in class_fields.names() {
138126
let key = KeyClassField(cls.index(), name.clone());
139127
if let Some(field) = self.get_from_class(cls, &key) {
140128
map.insert(name.clone(), field);

pyrefly/lib/alt/solve.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2366,7 +2366,9 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> {
23662366
let Some(owner) = class_binding.0.as_ref() else {
23672367
return;
23682368
};
2369-
if owner.contains(&expect.attr.id)
2369+
if self
2370+
.get_class_fields(owner)
2371+
.is_some_and(|f| f.contains(&expect.attr.id))
23702372
&& self.is_subset_eq(
23712373
&value_type,
23722374
&self.union(

pyrefly/lib/test/enums.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,10 @@ class E(enum.Enum):
2424
"#,
2525
);
2626
let cls = get_class("E", &handle, &state);
27-
let fields = cls
28-
.fields()
27+
let bindings = state.transaction().get_bindings(&handle).unwrap();
28+
let class_fields = bindings.get_class_fields(cls.index()).unwrap();
29+
let fields = class_fields
30+
.names()
2931
.map(|f| f.as_str())
3032
.sorted()
3133
.collect::<Vec<_>>();

0 commit comments

Comments
 (0)