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

Ensure DispatchedSet unique keys on construction #23

Merged
merged 1 commit into from
Mar 18, 2021
Merged

Conversation

charleskawczynski
Copy link
Owner

@charleskawczynski charleskawczynski commented Mar 18, 2021

This PR changes the behavior so that we error on the construction of DispatchedSets, rather than throwing an error on getindex(::DispatchedSet, key) for non-unique keys. I think I prefer this behavior, erroring on construction.

At first I tried using unique_elems, but this incurred runtime overhead. Then, I tried the generated function elems_are_unique, and that seems to keep things all at compile time 🚀!

I think it's still useful for users to have unique_elems / unique_keys(dt::AbstractDispatchedTuple), in case they may want to convert a DispatchedTuple into a DispatchedSet. I'll avoid exporting for now so that we can bikeshed the name / design.

@codecov
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #23 (87a36fb) into main (b8457c3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #23   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           63        78   +15     
=========================================
+ Hits            63        78   +15     
Impacted Files Coverage Δ
src/DispatchedTuples.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8457c3...87a36fb. Read the comment docs.

Copy link
Collaborator

@ali-ramadhan ali-ramadhan left a comment

Choose a reason for hiding this comment

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

It seems like using unique_elems in construction or getindex has performance implication but wouldn't it be a one-time cost in the constructor and a cost every call if it's in getindex?

@charleskawczynski
Copy link
Owner Author

It seems like using unique_elems in construction or getindex has performance implication but wouldn't it be a one-time cost ...?

Yes, unless it's repeatedly constructed.

and a cost every call if it's in getindex?

I think the getindex cost is purely compile-time.

@charleskawczynski
Copy link
Owner Author

After experimenting, I'm going to leave things as they were but still introduce unique_elems / unique_keys(dt::AbstractDispatchedTuple).

@charleskawczynski
Copy link
Owner Author

After further experimentation, I think we can get back to basically no runtime overhead using generated functions 😬.

@ali-ramadhan
Copy link
Collaborator

@generated is basically black magic to me lol

@charleskawczynski
Copy link
Owner Author

charleskawczynski commented Mar 18, 2021

@generated is basically black magic to me lol

I'm not sure I have the best understanding, but, my mental model consists of three fundamental pieces:

  1. @generated always return expressions
  2. @generated is an escape hatch where you can generate code only when you know information about variable types, but not values. So, AFAICT, generated function signature arguments might as well be foo(::TA, ::TB, ...) instead of foo(a::TA, b::TB, ...), because there is no a & b at the time that the generated function is first called (and the code is generated).
  3. @generated can be a burden on the compiler

@charleskawczynski
Copy link
Owner Author

I'm happy with these changes overall.

@charleskawczynski
Copy link
Owner Author

bors r+

@charleskawczynski charleskawczynski added enhancement New feature or request and removed Speculative labels Mar 18, 2021
@bors
Copy link
Contributor

bors bot commented Mar 18, 2021

@bors bors bot merged commit c4de321 into main Mar 18, 2021
@bors bors bot deleted the ck/unique_keys branch March 18, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants