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 pop! for AbstractSet and remove some implementations of pop! for concrete sets #49463

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LilithHafner
Copy link
Member

This is mostly a refactoring, but it also means that anyone who defines an AbstractSet in a package with delete! gets some pop! functions for free.

(The AbstractSet interface remains undocumented)

This PR would be bad if we want pop! to be what new types define and delete! to be defined in terms of pop! in general, but I prefer the other way around (and the other way around is how BitSet is already implemented)

@@ -101,8 +101,7 @@ function in!(x, s::Set)
end

push!(s::Set, x) = (s.dict[x] = nothing; s)
pop!(s::Set, x) = (pop!(s.dict, x); x)
pop!(s::Set, x, default) = (x in s ? pop!(s, x) : default)
Copy link
Member

Choose a reason for hiding this comment

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

I believe, pop! should be optimized for Set to hash only once. The problem comes from the fact that there is no token API to implement pop! hashing only once through AbstractSet (#24454). I have added a similar optimization into internal Base.in! (#45156).

Copy link
Member Author

Choose a reason for hiding this comment

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

That optimization is already present for pop!(s::Set, x). This PR keeps it that way.
You may want pop!(s::Set, x, default) to also have that optimization, but that doesn't justify keeping around a redundant method that does not have the optimization.

@LilithHafner
Copy link
Member Author

That said, we may want to make the AbstractSet API only require a single modify! function, which everything else is defined in terms of. In that case, this would not be a good PR to have merged.

@LilithHafner LilithHafner added the DO NOT MERGE Do not merge this PR! label Apr 23, 2023
@@ -19,8 +19,7 @@ isempty(s::IdSet) = isempty(s.dict)
length(s::IdSet) = length(s.dict)
in(@nospecialize(x), s::IdSet) = haskey(s.dict, x)
push!(s::IdSet, @nospecialize(x)) = (s.dict[x] = nothing; s)
pop!(s::IdSet, @nospecialize(x)) = (pop!(s.dict, x); x)
pop!(s::IdSet, @nospecialize(x), @nospecialize(default)) = (x in s ? pop!(s, x) : default)
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity. The fallback implementation pop!(s::AbstractSet, x, default) drops @nospecialize. Is there any significant difference?

Copy link
Member Author

Choose a reason for hiding this comment

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

The @nospecialize on x saves compile time and has no runtime cost (same @code_llvm output). I believe the same is the case for the @nospecialize on default. It seems that adding more specializations to the source code results in fewer specializations in the compiled code. This creates an interesting value judgement, I lean towards making no change i.e. don't delete these two pop! specializations.

julia> mypop1!(s::Base.IdSet, @nospecialize(x)) = (pop!(s.dict, x); x)
mypop1! (generic function with 1 method)

julia> mypop2!(s::Base.IdSet, x) = (pop!(s.dict, x); x)
mypop2! (generic function with 1 method)

julia> s = Base.IdSet(); push!.((s,), [:a, "b", 'c']); s
Base.IdSet{Any} with 3 elements:
  :a
  "b"
  'c'

julia> @time @eval mypop1!(s, :a)
  0.002980 seconds (1.46 k allocations: 108.076 KiB, 97.34% compilation time)
:a

julia> @time @eval mypop1!(s, "b")
  0.000059 seconds (44 allocations: 2.188 KiB)
"b"

julia> @time @eval mypop1!(s, 'c')
  0.000061 seconds (44 allocations: 2.188 KiB)
'c': ASCII/Unicode U+0063 (category Ll: Letter, lowercase)

julia> s = Base.IdSet(); push!.((s,), [:a, "b", 'c']); s
Base.IdSet{Any} with 3 elements:
  :a
  "b"
  'c'

julia> @time @eval mypop2!(s, :a)
  0.002935 seconds (1.44 k allocations: 107.107 KiB, 96.49% compilation time)
:a

julia> @time @eval mypop2!(s, "b")
  0.002701 seconds (1.43 k allocations: 106.529 KiB, 97.39% compilation time)
"b"

julia> @time @eval mypop2!(s, 'c')
  0.002824 seconds (1.43 k allocations: 106.529 KiB, 97.48% compilation time)
'c': ASCII/Unicode U+0063 (category Ll: Letter, lowercase)

@DilumAluthge DilumAluthge marked this pull request as draft June 24, 2023 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Do not merge this PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants