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

Use DenseDict for copy_to #1122

Merged
merged 2 commits into from
Jul 12, 2020
Merged

Use DenseDict for copy_to #1122

merged 2 commits into from
Jul 12, 2020

Conversation

blegat
Copy link
Member

@blegat blegat commented Jul 10, 2020

If no variable was deleted, the variable indices are contiguous from 1 to n so we can store the mapping in a Vector instead of a Dict. This PR allows to exploit it.
The field of IndexMap is now a Union of 2 concrete types. This gives the cost of a if when accessing the field which is smaller than the cost of hashing an index. Moreover, if the index map is going to be accessed a lot, a function barrier can be used to even avoid the cost of this if.

@joaquimg
Copy link
Member

Should we use the same code as in CleverDicts?

@blegat
Copy link
Member Author

blegat commented Jul 10, 2020

The use case is a bit different. Here, even if we know the variable indices are from 1 to n, we may not copy the variables in this order so we may not set the dictionary in the same order. Therefore, we preallocate the whole vector but then haskey is different from CleverDict as here the Vector has undef for unset keys so we need to add a BitSet to be able to implement haskey correctly.

src/Utilities/dense_dict.jl Show resolved Hide resolved
Base.length(d::DenseDict) = length(d.set)
Base.haskey(dict::DenseDict, key) = dict.hash(key) in dict.set
Base.getindex(dict::DenseDict, key) = dict.map[dict.hash(key)]
function Base.setindex!(dict::DenseDict, value, key)
Copy link
Member

Choose a reason for hiding this comment

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

what happens if one goes beyond the limit?
should it resize automatically? or even use sizehint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently it's an error, we can revisit once we have a use case for this

@blegat blegat changed the title [RFC] Use DenseDict for copy_to Use DenseDict for copy_to Jul 12, 2020
@blegat blegat merged commit 21d3dc4 into master Jul 12, 2020
@blegat blegat added this to the v0.9.15 milestone Jul 24, 2020
@odow odow deleted the bl/dense_dict branch September 2, 2020 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants