Skip to content

Ale/3.0 optimize pat#262

Merged
0x0f0f0f merged 7 commits intoale/3.0from
ale/3.0-optimize-pat
Feb 24, 2025
Merged

Ale/3.0 optimize pat#262
0x0f0f0f merged 7 commits intoale/3.0from
ale/3.0-optimize-pat

Conversation

@0x0f0f0f
Copy link
Member

@0x0f0f0f 0x0f0f0f commented Feb 5, 2025

Found some time to finish an optimization I had lying around. Use an optimized struct for Patterns for type stability of most recursive procedures.

@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 93.12977% with 18 lines in your changes missing coverage. Please review.

Project coverage is 82.28%. Comparing base (32786c0) to head (4191320).

Files with missing lines Patch % Lines
src/Patterns.jl 88.31% 9 Missing ⚠️
src/Rules.jl 78.78% 7 Missing ⚠️
src/Syntax.jl 96.00% 1 Missing ⚠️
src/vecexpr.jl 50.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           ale/3.0     #262      +/-   ##
===========================================
+ Coverage    81.42%   82.28%   +0.86%     
===========================================
  Files           19       19              
  Lines         1491     1547      +56     
===========================================
+ Hits          1214     1273      +59     
+ Misses         277      274       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented Feb 5, 2025

Benchmark Results

egg-sym egg-cust MT@41913204571... MT@32786c06710... egg-sym/MT@419... egg-cust/MT@41... MT@32786c06710...
egraph_addexpr 1.41 ms 5 ms 5.07 ms 0.283 1.01
basic_maths_simpl2 14.8 ms 5.32 ms 21.2 ms 21.8 ms 0.7 0.251 1.03
prop_logic_freges_theorem 2.56 ms 1.57 ms 2.26 ms 2.37 ms 1.13 0.695 1.05
calc_logic_demorgan 61.6 μs 34.9 μs 78.7 μs 80.8 μs 0.782 0.443 1.03
calc_logic_freges_theorem 22.1 ms 12.2 ms 39.7 ms 40.8 ms 0.558 0.308 1.03
basic_maths_simpl1 6.47 ms 2.9 ms 4.52 ms 4.81 ms 1.43 0.642 1.06
egraph_constructor 0.0833 μs 0.0959 μs 0.0946 μs 0.869 0.987
prop_logic_prove1 35.6 ms 14.1 ms 38.2 ms 39 ms 0.932 0.369 1.02
prop_logic_demorgan 80.8 μs 45.5 μs 93.2 μs 94.4 μs 0.868 0.488 1.01
while_superinterpreter_while_10 18.2 ms 18.7 ms 1.03
prop_logic_rewrite 110 μs 114 μs 1.03
time_to_load 117 ms 118 ms 1.01

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@0x0f0f0f 0x0f0f0f mentioned this pull request Feb 15, 2025
@inline get_constant(@nospecialize(g::EGraph), hash::UInt64) = g.constants[hash]
@inline has_constant(@nospecialize(g::EGraph), hash::UInt64)::Bool = haskey(g.constants, hash)

@inline needs_operation_quoting(g::EGraph) = false
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO document

@assert isground(p)
function lookup_pat(g::EGraph{ExpressionType}, p::Pat)::Id where {ExpressionType}
if p.type === PAT_LITERAL
# TODO avoid using enode vector for lookup, just try using hash
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @gkronber this is related to how literals are handled in the e-graph. we could probably save some computation time here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how. We need to return the eclass-id for the literal, and for this we need to find the node in memo and then find the canonical eclass id via UF.
The canonicalization of VecExpr (in lookup()) is not necessary for literal enodes, but probably not too costly.

Comment on lines +45 to 54
function cached_ids(g::EGraph, p::Pat)
p.type === PAT_VARIABLE && return Iterators.map(x -> x.val, keys(g.classes))

if p.isground
id = lookup_pat(g, p)
iszero(id) ? UNDEF_ID_VEC : [id]
id > 0 ? [id] : UNDEF_ID_VEC
else
get(g.classes_by_op, IdKey(v_signature(p.n)), UNDEF_ID_VEC)
end
end
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO not type stable

Abstract type representing a pattern used in all the various pattern matching backends.
"""
abstract type AbstractPat end
@enum PatternType PAT_LITERAL PAT_VARIABLE PAT_SEGMENT PAT_EXPR
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO document

@0x0f0f0f
Copy link
Member Author

@gkronber this should remove the runtime dispatch overhead in instantiate_enode!. Let's wait for the benchmark results. Although the enodes are still instantiate recursively, we could find a way of having an imperative (stack based? generated?) way of unrolling enode instantiation to make it even faster, or avoid re-adding enodes for literals.

The only runtime dispatch overhead that should be left is in eqsat_search! when calling rule.ematcher_left! (or right), since the function is created by the macro and is a field of a struct.

A well known problem

https://discourse.julialang.org/t/struct-with-a-field-of-type-function/49669/5
https://discourse.julialang.org/t/type-stability-when-having-function-as-struct-field/84838

While Function wrappers may not work, I would be ok with a @ccall-like low level hack since we always know the signature of the generated ematcher functions in a subsequent MR

@0x0f0f0f
Copy link
Member Author

JET.report_package(Metatheory) doesn't show any major problems anymore

@gkronber
Copy link
Collaborator

4191320 LGTM

@0x0f0f0f 0x0f0f0f merged commit 57bfe22 into ale/3.0 Feb 24, 2025
4 checks passed
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.

3 participants