Skip to content
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.

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

Add hash field to all BasicSymbolic subtypes #590

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bowenszhu
Copy link
Member

@bowenszhu bowenszhu commented Apr 30, 2024

This PR does the following things:

  • Previously only Term, Mul, Add have the field hash. It is now added to all BasicSymbolic subtypes. Now hash is only computed and stored the first time when it is needed.
    Fix Why don't Pow and Div have hash field? #465

  • Previously the default initial value of the field hash is const EMPTY_HASH = RefValue(UInt(0)). That is, the pointer of the hash field of any newly constructed BasicSymbolic directs to exactly the identical RefValue(UInt(0)) object. This has not caused issues because the constructors of Term, Mul, Add manually created a fresh RefValue(UInt(0)). This PR modifies the struct definition of BasicSymbolic such that it automatically creates a new RefVaule object for hash whenever a new BasicSymbolic object is created and we don't need to add it as a keyword argument anymore in the constructors.

  • A test is fixed, because that as a field of type RefValue{UInt} is added to Sym, the following should now return false.

using SymbolicUtils
a1 = only(@syms a)
a2 = only(@syms a)
a1 === a2 # false

Copy link
Contributor

github-actions bot commented Apr 30, 2024

Benchmark Results

master 8d8bd28... master/8d8bd28924b323...
overhead/acrule/a+2 0.718 ± 0.018 μs 0.729 ± 0.016 μs 0.985
overhead/acrule/a+2+b 0.719 ± 0.019 μs 0.721 ± 0.017 μs 0.997
overhead/acrule/a+b 0.254 ± 0.013 μs 0.263 ± 0.013 μs 0.965
overhead/acrule/noop:Int 26.2 ± 0.051 ns 25 ± 0.92 ns 1.05
overhead/acrule/noop:Sym 0.0342 ± 0.0049 μs 0.0333 ± 0.0058 μs 1.03
overhead/rule/noop:Int 0.0371 ± 0.00038 μs 0.0368 ± 0.00037 μs 1.01
overhead/rule/noop:Sym 0.0421 ± 0.0014 μs 0.0421 ± 0.0013 μs 0.999
overhead/rule/noop:Term 0.0421 ± 0.0014 μs 0.0426 ± 0.0014 μs 0.99
overhead/ruleset/noop:Int 0.122 ± 0.0027 μs 0.122 ± 0.0026 μs 0.996
overhead/ruleset/noop:Sym 0.137 ± 0.0034 μs 0.135 ± 0.0034 μs 1.01
overhead/ruleset/noop:Term 6.58 ± 0.42 μs 6.66 ± 0.47 μs 0.987
overhead/simplify/noop:Int 0.146 ± 0.0021 μs 0.138 ± 0.0058 μs 1.06
overhead/simplify/noop:Sym 0.156 ± 0.005 μs 0.16 ± 0.0025 μs 0.972
overhead/simplify/noop:Term 0.044 ± 0.0027 ms 0.0449 ± 0.0025 ms 0.979
overhead/simplify/randterm (+, *):serial 0.132 ± 0.0032 s 0.132 ± 0.0024 s 0.997
overhead/simplify/randterm (+, *):thread 0.0845 ± 0.026 s 0.0822 ± 0.028 s 1.03
overhead/simplify/randterm (/, *):serial 0.267 ± 0.0096 ms 0.267 ± 0.011 ms 1
overhead/simplify/randterm (/, *):thread 0.315 ± 0.012 ms 0.325 ± 0.011 ms 0.97
overhead/substitute/a 0.123 ± 0.0039 ms 0.122 ± 0.0034 ms 1
overhead/substitute/a,b 0.102 ± 0.0031 ms 0.103 ± 0.0034 ms 0.987
overhead/substitute/a,b,c 17.5 ± 0.78 μs 16.6 ± 0.74 μs 1.05
polyform/easy_iszero 0.0533 ± 0.0024 ms 0.0529 ± 0.0025 ms 1.01
polyform/isone 3.1 ± 0.009 ns 2.79 ± 0.01 ns 1.11
polyform/iszero 5.5 ± 0.093 ms 5.54 ± 0.089 ms 0.993
polyform/simplify_fractions 3.34 ± 0.057 ms 3.32 ± 0.054 ms 1
time_to_load 4.54 ± 0.0032 s 4.55 ± 0.025 s 0.999

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).

@bowenszhu bowenszhu marked this pull request as draft May 1, 2024 07:13
`===` is expected to return `false` for different `Sym` instances even if their names are the same
@bowenszhu bowenszhu marked this pull request as ready for review May 1, 2024 21:38
@shashi shashi requested a review from YingboMa May 1, 2024 22:26
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.

Why don't Pow and Div have hash field?
1 participant