Add IdSet and fix IdDict performance problem #26054
Conversation
base/abstractdict.jl
Outdated
|
|
||
| filter!(f, d::IdSet) = unsafe_filter!(f, d) | ||
|
|
||
| function union!(s::IdSet{T}, itr) where T |
There was a problem hiding this comment.
The existing copy of this method should probably be expanded to AbstractSet instead.
Keno
force-pushed
the
kf/idset
branch
2 times, most recently
from
843a2cf
to
cbc0be9
Compare
|
CircleCI seems to have started running out of memory, but other than that, this seems good. @JeffBezanson any further review? |
|
Can we get some local performance numbers? From Travis logs, it looks like this just over doubled the build time and increased the size by ~20-30% for the compiler, but those numbers aren't usually particularly reliable. |
|
Alright, well, this seems to be a pre-req for #26079 now. So, bump? |
|
Was just int he process of rebasing and looking at performance. |
To provide an efficient `length` method. Fixes #26043
|
I see about a 1.3s increase in time to build basecompiler.ji (about 5%) and a 1% increase in basecompiler size. I think that's ok. |
|
nanosoldier saw performance regressions in bitset on yesterday's nightly run (https://github.com/JuliaCI/BaseBenchmarkReports/blob/656af6a7f0e5c93b5e0fa03c9e412307c3e46531/daily_2018_2_25/report.md). This seems like the most likely candidate. Investigating. |
In #26054, I generalized `Set`'s union! method to apply to AbstractSet. Set's union! method tries to optimize common cases where the number of possible values of the set type are small. However, to do this, it repeatedly calls the set's length method. This is bad for `BitSet` for two reasons: 1. Its `length` method is O(N) in the number of elements. 2. The check isn't actually useful, since the eltype of `BitSet` is always Int and it's impossible to have a BitSet that already contains all integers. This simply special cases `union!` on BitSet to use the same implementation it did previously.
|
|
|
As long as we're confident that the API is what we want, I can't see why we wouldn't export it. |
https://github.com/Keno/NewOptimizer.jl requires various sets and dicts. Since those are currently not available at inference time, I'm changing it to use
IdDicts. However, until now there was noSetanalogue forIdDict, so this PR provides that. Additionally, this fixes #26043, which was causing disastrous performance when using IdDicts rather than Dicts in the new optimizer.