Fix hashing and memoization of enodes (VecExpr) #239

gkronber-machine · 2024-08-24T09:37:45Z

This new PR fixes an issue with memoization of enodes, whereby enodes in the memo dictionary are updated, invalidating their hash value.
Additional fixes and improvements:

Fix the check_memo() function to actually check memo
Fix the implementation of Base.hash for VecExpr
Improve efficiency of dictionary lookups by using get() / get!()

(related PR: #238 has additional changes to remove caching of hash values for VecExpr)

0x0f0f0f · 2024-08-24T14:39:21Z

src/EGraphs/egraph.jl

+  # orig = copy(n)
+  # inmemo = any(entry -> objectid(entry) == objectid(n), keys(g.memo))


@gkronber leftover

src/EGraphs/uniquequeue.jl

src/vecexpr.jl

codecov-commenter · 2024-08-24T18:48:27Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.06%. Comparing base (1dc53da) to head (66ea780).

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

Additional details and impacted files

@@             Coverage Diff             @@
##           ale/3.0     #239      +/-   ##
===========================================
+ Coverage    80.78%   81.06%   +0.28%     
===========================================
  Files           19       19              
  Lines         1504     1500       -4     
===========================================
+ Hits          1215     1216       +1     
+ Misses         289      284       -5

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

0x0f0f0f · 2024-08-25T11:40:28Z

@gkronber it it provides a substantial speedup, we could move the UniqueQueue to another PR and merge it when Julia 1.11 is released, setting the min version of Julia to it.

gkronber · 2024-08-25T12:56:46Z

@gkronber it it provides a substantial speedup, we could move the UniqueQueue to another PR and merge it when Julia 1.11 is released, setting the min version of Julia to it.

In local benchmarks it shows almost no difference.

0x0f0f0f · 2024-08-26T14:27:39Z

@gkronber it it provides a substantial speedup, we could move the UniqueQueue to another PR and merge it when Julia 1.11 is released, setting the min version of Julia to it.

In local benchmarks it shows almost no difference.

Could you post it here please?

0x0f0f0f · 2024-08-26T19:54:54Z

@gkronber it it provides a substantial speedup, we could move the UniqueQueue to another PR and merge it when Julia 1.11 is released, setting the min version of Julia to it.

In local benchmarks it shows almost no difference.

Could you post it here please?

(@gkronber I mean this branch against ale/3.0)

src/EGraphs/egraph.jl

gkronber · 2024-08-27T14:31:35Z

Local benchmark results (66ea780):

julia> include("benchmark/benchmarks.jl")
Precompiling Metatheory
  1 dependency successfully precompiled in 2 seconds. 6 already precompiled.
Benchmark(evals=1, seconds=5.0, samples=10000)

julia> run(SUITE)
5-element BenchmarkTools.BenchmarkGroup:
  tags: []
  "prop_logic" => 4-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraph", "logic"]
          "freges_theorem" => Trial(1.101 ms)
          "prove1" => Trial(39.136 ms)
          "demorgan" => Trial(96.700 μs)
          "rewrite" => Trial(113.200 μs)
  "basic_maths" => 2-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraphs"]
          "simpl2" => Trial(20.215 ms)
          "simpl1" => Trial(4.877 ms)
  "while_superinterpreter" => 1-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraph"]
          "while_10" => Trial(18.184 ms)
  "egraph" => 2-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraphs"]
          "addexpr" => Trial(5.435 ms)
          "constructor" => Trial(500.000 ns)
  "calc_logic" => 2-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraph", "logic"]
          "freges_theorem" => Trial(38.887 ms)
          "demorgan" => Trial(79.000 μs)

ale/3.0 branch (1dc53da):

1 dependency successfully precompiled in 2 seconds. 6 already precompiled.
Benchmark(evals=1, seconds=5.0, samples=10000)

julia> run(SUITE)
5-element BenchmarkTools.BenchmarkGroup:
  tags: []
  "prop_logic" => 4-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraph", "logic"]
          "freges_theorem" => Trial(1.070 ms)
          "prove1" => Trial(35.801 ms)
          "demorgan" => Trial(97.100 μs)
          "rewrite" => Trial(115.000 μs)
  "basic_maths" => 2-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraphs"]
          "simpl2" => Trial(18.172 ms)
          "simpl1" => Trial(4.460 ms)
  "while_superinterpreter" => 1-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraph"]
          "while_10" => Trial(18.715 ms)
  "egraph" => 2-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraphs"]
          "addexpr" => Trial(5.079 ms)
          "constructor" => Trial(500.000 ns)
  "calc_logic" => 2-element BenchmarkTools.BenchmarkGroup:
          tags: ["egraph", "logic"]
          "freges_theorem" => Trial(35.986 ms)
          "demorgan" => Trial(75.800 μs)

0x0f0f0f · 2024-08-27T16:50:32Z

Amazing. Thank you @gkronber - Merging when you mark this as ready 👍

gkronber · 2024-08-28T14:37:51Z

I have a few more changes that I would like to sneak into this PR. They are only tangentially related. @0x0f0f0f, can I add the changes here or should I create a new PR for those?

0x0f0f0f · 2024-08-28T14:56:03Z

I have a few more changes that I would like to sneak into this PR. They are only tangentially related. @0x0f0f0f, can I add the changes here or should I create a new PR for those?

Another MR would be better, let's merge this one first

gkronber · 2024-08-31T11:09:17Z

I saw that the MT code deviates from most recent egg code slightly since the changes from egraphs-good/egg#291 which is partially related to this PR and mentions There are also memory usage/performance advantages since EClass.parents, EGraph.pending, and EGraph.analysis_pending, no longer need to store nodes.

It should be straight-forward to make the corresponding changes to MT, I'd be happy to prepare a PR for this.
@0x0f0f0f, is there some work in this direction already ?

0x0f0f0f · 2024-09-03T10:17:09Z

I saw that the MT code deviates from most recent egg code slightly since the changes from egraphs-good/egg#291 which is partially related to this PR and mentions There are also memory usage/performance advantages since EClass.parents, EGraph.pending, and EGraph.analysis_pending, no longer need to store nodes.

It should be straight-forward to make the corresponding changes to MT, I'd be happy to prepare a PR for this. @0x0f0f0f, is there some work in this direction already ?

Some stuff was done in #223 ! But interesting PR, I'll check it out. Let's merge this one first

0x0f0f0f · 2024-09-03T10:39:09Z

@gkronber I gave you maintainer write access, you should now be able to push to branches of this repository, so that benchmarks will run automatically on your PRs, if you open them from branches of this repo (and not from forks)

:)

gkronber · 2024-09-03T14:11:21Z

Great, that should simplify things.

gkronber added 14 commits August 23, 2024 10:51

Implement the correct hash function.

b4df50b

Fix upwards update of semantic analysis values.

f987d66

Remove unused code

313c5e8

Fix check_memo()

f14229d

Fix an issue where enodes are not added to memo.

6b6522e

Simplify code

84d2e30

Add code to make sure we do not canonicalize vecexpr in memo

7adb07e

Make sure nodes in dictionaries (memo and pending) are cloned

9b3d0e4

Use in!() instead of in() for UniqueQueue push!()

6c7a56a

Comment runtime assertions

a4fe7a2

Merge remote-tracking branch 'origin/ale/3.0' into fix_enode_memo_2

b3bcd83

Fix function call in in!()

d916a0d

Remove unnecessary copies of VecExpr

d5dce5d

Improve add_class_by_op to use a single dictionary lookup

fbdfc73

0x0f0f0f reviewed Aug 24, 2024

View reviewed changes

src/EGraphs/uniquequeue.jl Outdated Show resolved Hide resolved

0x0f0f0f reviewed Aug 24, 2024

View reviewed changes

src/vecexpr.jl Show resolved Hide resolved

gkronber added 2 commits August 24, 2024 20:40

Removed old commented code.

f74d552

Move in!() outside of push!()

a3ebcb6

Revert change in UniqueQueue as it does not work with Julia v1.8

66ea780

0x0f0f0f reviewed Aug 26, 2024

View reviewed changes

src/EGraphs/egraph.jl Show resolved Hide resolved

0x0f0f0f reviewed Aug 26, 2024

View reviewed changes

src/EGraphs/egraph.jl Show resolved Hide resolved

gkronber mentioned this pull request Aug 28, 2024

Fix enode memoization #238

Closed

gkronber-machine marked this pull request as ready for review August 29, 2024 18:10

0x0f0f0f merged commit 0b2e6c9 into JuliaSymbolics:ale/3.0 Sep 3, 2024
3 of 4 checks passed

This was referenced Oct 7, 2024

Don't copy if should_copy is false #251

Closed

Performance improvements #253

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hashing and memoization of enodes (VecExpr) #239

Fix hashing and memoization of enodes (VecExpr) #239

gkronber-machine commented Aug 24, 2024

0x0f0f0f Aug 24, 2024

codecov-commenter commented Aug 24, 2024 •

edited

Loading

0x0f0f0f commented Aug 25, 2024

gkronber commented Aug 25, 2024

0x0f0f0f commented Aug 26, 2024

0x0f0f0f commented Aug 26, 2024

gkronber commented Aug 27, 2024

0x0f0f0f commented Aug 27, 2024

gkronber commented Aug 28, 2024

0x0f0f0f commented Aug 28, 2024

gkronber commented Aug 31, 2024

0x0f0f0f commented Sep 3, 2024

0x0f0f0f commented Sep 3, 2024 •

edited

Loading

gkronber commented Sep 3, 2024

		# orig = copy(n)
		# inmemo = any(entry -> objectid(entry) == objectid(n), keys(g.memo))

Fix hashing and memoization of enodes (VecExpr) #239

Fix hashing and memoization of enodes (VecExpr) #239

Conversation

gkronber-machine commented Aug 24, 2024

0x0f0f0f Aug 24, 2024

Choose a reason for hiding this comment

codecov-commenter commented Aug 24, 2024 • edited Loading

Codecov Report

0x0f0f0f commented Aug 25, 2024

gkronber commented Aug 25, 2024

0x0f0f0f commented Aug 26, 2024

0x0f0f0f commented Aug 26, 2024

gkronber commented Aug 27, 2024

0x0f0f0f commented Aug 27, 2024

gkronber commented Aug 28, 2024

0x0f0f0f commented Aug 28, 2024

gkronber commented Aug 31, 2024

0x0f0f0f commented Sep 3, 2024

0x0f0f0f commented Sep 3, 2024 • edited Loading

gkronber commented Sep 3, 2024

codecov-commenter commented Aug 24, 2024 •

edited

Loading

0x0f0f0f commented Sep 3, 2024 •

edited

Loading