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 LittleDict #19

Merged
merged 19 commits into from
Mar 29, 2019
Merged

Add LittleDict #19

merged 19 commits into from
Mar 29, 2019

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Mar 20, 2019

This PR adds a new type LittleDict.
It is optized for small numbers of elements
My timing on a earlier version* had it hitting parity with OrderedDict for Symbol keys, at 30 elements when backed with a Vector or 50 elements when backed with a Tuple.

I would like to put it in OrderedCollections.jl because

  • A) it is an ordered collection
  • B) many uses of OrderedDict right now, would be better off using LittleDict. (Basically all my . previous uses. Though I may be a nonrepresentative sample.)

TODO is tests, and a bit more API, esp constructors.
But I will let the tests driive those.
I want to basically reuse the OrderedDict tests, (except the ones that involve 10,000 elements)
but I am undecided as to if that should be via copy-paste,
or via adding abstraction to the current testset.

  • (Timing done on a version that was 30% slower than the one in this PR)

src/little_dict.jl Outdated Show resolved Hide resolved
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

I would approve having this here.

src/dict_sorting.jl Show resolved Hide resolved
src/dict_sorting.jl Outdated Show resolved Hide resolved
src/little_dict.jl Outdated Show resolved Hide resolved
src/little_dict.jl Outdated Show resolved Hide resolved
src/little_dict.jl Show resolved Hide resolved
src/little_dict.jl Outdated Show resolved Hide resolved
src/little_dict.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Member Author

@timholy do you think I should abstract the OrderedDict tests, to be parameterised by type,
or copy and paste and edit?

@oxinabox
Copy link
Member Author

Ok, I am done.
This is now ready for review.

The tests on the Frozen LittleDict are rather simple, but I think it is enough.

I think there is a bit more that can be done to make FrozenLittleDicts a bit faster,
and that would warrent more tests.
And aI think a trait or two (KeyFrozen <: Frosty, FullyFrozen <: Frosty, NotFrozen)
But all that can come in another PR if someone wants it.

@oxinabox
Copy link
Member Author

I microoptimized setindex! with some careful benchmarking.
A overall ~30% speedup.
The biggest part of which was dropping the @assert.
Dropping @simd ended up changing nothing,
I guess the stuff I was doing before to make @simd work
like changing from interator to index was what really gave the speedup not the @simd itself.

I don't care so much about the performance of getindex so I have not done the same benchmarking and microoptimisations for get.
But the same logic should hold from dropping SIMD.

Copy link
Member

@kmsquire kmsquire left a comment

Choose a reason for hiding this comment

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

Looks good to me. Left some minor comments.

src/little_dict.jl Show resolved Hide resolved
src/little_dict.jl Show resolved Hide resolved
test/test_little_dict.jl Show resolved Hide resolved
test/test_little_dict.jl Show resolved Hide resolved
@timholy
Copy link
Member

timholy commented Mar 26, 2019

Once the length check is in or argued to be unnecessary, I'm good with this, no need for further review.

@oxinabox
Copy link
Member Author

Length check addded,

I got enthused and added better use of the type system to ensure we never tried to mutate unmutable things.
Which then let me remove some exception handling.

The tests don't test the unhappy path very well.
(but I think they do test it some).

Copy link
Member

@kmsquire kmsquire left a comment

Choose a reason for hiding this comment

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

One small change needed for tests to pass on 32-bit systems.

test/test_little_dict.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Member Author

Ready for merge

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.

5 participants