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

Implement hash consing for Sym #658

Merged
merged 20 commits into from
Nov 7, 2024
Merged

Implement hash consing for Sym #658

merged 20 commits into from
Nov 7, 2024

Conversation

bowenszhu
Copy link
Member

@bowenszhu bowenszhu commented Oct 14, 2024

This PR implements hash consing for Sym as the first step towards applying hash consing across SymbolicUtils.jl.

Motivation:

Hash consing enables efficient memory usage and potential performance improvements by ensuring that only one unique copy of a symbolic expression exists in memory.

Implementation Details:

  • A WeakValueDict is used to store existing symbolic objects. This choice ensures that symbolic objects without strong references are eligible for garbage collection.
  • The BasicSymbolic struct is modified to be mutable to comply with the requirements of WeakValueDict (due to how Julia implements finalizers).
  • A new "hash" function, hash2, is introduced to incorporate symtype into the hash calculation. This approach was chosen over modifying the existing hash function to avoid breaking many tests because ordering is dependent on hash values.
  • A new isequal2 function is added for metadata comparison in addition to Base.isequal, because directly modifying Base.isequal breaks numerous tests in ModelingToolkit.jl.

Future PRs will apply hash consing to other BasicSymbolic subtypes.

@bowenszhu bowenszhu marked this pull request as draft October 14, 2024 21:22
@bowenszhu bowenszhu changed the title Implement hash consing Implement hash consing for Sym Oct 23, 2024
@bowenszhu bowenszhu marked this pull request as ready for review October 23, 2024 05:17
@ChrisRackauckas
Copy link
Member

Looks reasonable enough and the implementation is clean. Are there performance benchmarks to see if it's going on the right direction?

@AayushSabharwal
Copy link
Contributor

Considering this matches hashes to symbolic variables, wouldn't it also need to handle hash collisions? If two Syms end up with the same hash, it would return a different one than what the user expects.

Also, if hashconsing is using a custom hash function could it also hash the metadata and not require that it be empty?

src/types.jl Outdated
function Sym{T}(name::Symbol; metadata = NO_METADATA, kw...) where {T}
if metadata==NO_METADATA
s = Sym{T}(; name, kw...)
get!(wvd, hash2(s), s)
Copy link
Member

Choose a reason for hiding this comment

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

We need a dictionary that takes both hash and equal to handle collision.

@bowenszhu bowenszhu marked this pull request as ready for review November 5, 2024 18:06
@bowenszhu
Copy link
Member Author

requesting review @AayushSabharwal @ChrisRackauckas

src/types.jl Outdated Show resolved Hide resolved
src/types.jl Outdated Show resolved Hide resolved
@ChrisRackauckas ChrisRackauckas merged commit a587847 into master Nov 7, 2024
10 of 12 checks passed
@ChrisRackauckas ChrisRackauckas deleted the hash-consing branch November 7, 2024 16:10
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