Skip to content

Add LittleDict#19

Merged
kmsquire merged 19 commits intomasterfrom
ox/littledict
Mar 29, 2019
Merged

Add LittleDict#19
kmsquire merged 19 commits intomasterfrom
ox/littledict

Conversation

@oxinabox
Copy link
Copy Markdown
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)

Copy link
Copy Markdown
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.

@oxinabox
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@timholy
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@oxinabox
Copy link
Copy Markdown
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