Skip to content

Conversation

@Keno
Copy link
Member

@Keno Keno commented Mar 4, 2022

As mentioned in #44402, widenconst on a Const whose value is a Type can be quite slow - slow enough to show up noticeably in profiles. This attempts to address that by splitting the Const lattice element into two: Const and ConstType, the former being used for non-Type values and the latter being used for Types, additionally tracking the Type{T} for the constant type T, thus saving the lookup on widenconst.

Now, not every Const actually passes through widenconst, which would cause a performance regression, but it turns out that most ConstTypes originate from global references. Thus, we can take advantage of the recently added binding type field to cache the Type{T} value for bindings and directly feed that into ConstType.

As a second enhancement, when we don't already have a Type{T} value, we could make the corresponding ConstType field lazily computed on the first widenconst. However, because this would requires making ConstType mutable, this change is a bit of a wash performance wise. I included it in the anticipation that codegen for mutable types with const fields will improve in the future, but it's optional.

Keno added 3 commits March 4, 2022 08:46
Introduces a spearate lattice elements for tracking const Types
that also stores the corresponding Type{} information, ensuring
that this is only computed once and whenever possible is taken
from an existing instance of it instead of recomputing it.

Part of #44402
@Keno
Copy link
Member Author

Keno commented Mar 4, 2022

Oh I guess I forgot the punch line. This makes inference 20% faster on my particular inference benchmark.

(= (outerref ,name) ,name)))
(= (outerref ,name) ,name))
(block
(call (core set_binding_type!) (thismodule) (inert ,name) (call (core apply_type) (core Type) ,name))
Copy link
Member

Choose a reason for hiding this comment

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

@vtjnash
Copy link
Member

vtjnash commented Aug 31, 2023

IIUC, this was essentially replaced by #44704, before this one became badly conflicted

@vtjnash vtjnash closed this Aug 31, 2023
@vtjnash vtjnash deleted the kf/splitconst branch August 31, 2023 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants