Static Bitsets #647
Static Bitsets #647
Conversation
|
Should be ready for review. Will squash once reviews are done. |
| @inline rand(rng::Random.AbstractRNG, ::Random.SamplerType{SBitSet{N}}) where {N} = SBitSet(ntuple(i->rand(rng, UInt64), Val{N}())) | ||
|
|
||
|
|
||
| ## SIMD opt out. Make less ugly once github.com/JuliaLang/julia/pull/31113 has aged. |
There was a problem hiding this comment.
The entire simd opt-out is super fishy. It is also liable to break on new julia versions, since it relies on unexported internals, and the 1.0 vs 1.1-1.3 story is not pretty.
The opt-out is necessary when using code bases that assume that their input is @simd-able. The prime example is LightGraphs, that contains many @simd for node2 in neighbors(g, node1), but otherwise requires no random access to neighbors(g, node). Without a pragmatic opt-out, it would be impossible to use lightgraphs algorithms on custom graph types that use sbitset for neighbors.
Comments?
| mutable struct MBitSet{N} | ||
| chunks::NTuple{N, UInt64} | ||
| end | ||
|
|
There was a problem hiding this comment.
It would be tempting to use NTuple{N, VecElement{UInt64}} here. Perf with plain UInt64 looks ok, though. Diverging opinions?
There was a problem hiding this comment.
From memory (and I hope I'm remembering this right!), when I started this work, LLVM vectors of odd lengths (beginning at ~7?) wouldn't work... does anyone know whether they are safe/efficient for arbitrary N these days?
There was a problem hiding this comment.
LLVM vectors of odd lengths (beginning at ~7?) wouldn't work...
In these cases julia is smart enough to emit a working fallback llvm array (like NTuple{N, UInt64}). So that is not blocking.
On the other hand, NTuple{N, VecElement{UInt64}} is inconvenient to use, and llvm should optimize the array to vector anyways.
So we could probably do either and be OK.
| m = MBitSet(a) | ||
| m[idx] = b | ||
| return SBitSet(m) | ||
| end |
There was a problem hiding this comment.
SArray uses an ifelse construction instead of pointer arithmetic. LLVM is capable of removing the allocation from the MBitSet, though.
Is this approach ok or should I adopt the SArray solution?
There was a problem hiding this comment.
Since the compiler is getting better, I would go the other way - the SArray implementation could quite probably be simplified.
There was a problem hiding this comment.
I think it depends. Hell, even if I were writing this in assembly I wouldn't know whether to prefer ifelse or pointerset: ifelse is probably faster if the SArray fits into a register. On the other hand, the fact that pointerset-style works allocation-free is a somewhat recent development in the compiler, so it is no wonder that StaticArrays uses ifelse.
I guess I should run some benchmarks...
All of this makes it sound to me that this should perhaps be a separate package. |
Yeah, I'm somewhat sympathetic to that point of view as well. @chethega does the implementation here depend on Pragmatically, a new package that focuses on new containers with static (i.e. compile-time) guarantees could claim a Don't get me wrong - I think this is really good work :) It's just that part of the challenge/success here has been in minimizing the "surface area" of |
|
From a purely technical standpoint you're correct that this should be a separate package; there is literally zero code-reuse between SBitSet and StaticArrays, and SBitSet would not depend on StaticArrays. I wrote SBitSet for a separate project without relying on StaticArrays, and simply thought that this is the best place for open-sourcing SBitSet. From an ecosystem standpoint, tiny packages like a stand-alone SBitSet have a hard time of surviving: Discoverability is too hard, it would rely on a single person (me) being responsive as a maintainer forever, and the overhead for "amount of dependencies" doesn't just scale with SLOC, but also with plain "number of separate packages / maintainers" (both for security and for assurance of timely patches when something breaks). I'm not saying that everything should be in monolithic giant packages, just that there is a middle-ground involving a little bit of "grab-bagginess" that is better than leftpad-style micro-packages. StaticArrays is definitely one of the first places where people would look for SBitSet-like functionality. But if you think that it is better to split, then we can make SBitSet its own package, possibly under the umbrella of JuliaArrays, and possibly linked in the readme. We can then also establish a maintenance fallback (someone gets write access, such that downstream is not left in the rain if I mess up and you can worst-case steal the repo from me).
Thanks for the review! |
Considering that you'd like
I'm fine with it being in |
I need SBitSet as keys in homogeneous dictionaries. This means that I need fast hashing. If we implement This is the main issue that prevents any Base interface from being used. If you have any comments on that, please shoot away. My thinking on the remaining idiosyncracies was that if we depart from Base in terms of hashing and equality, then we can just go on and implement the most convenient syntax for everything (iteration is over set-elements,
Unless you have a magic solution to the hashing problem, or any of you, Kristoffer or Andy has a change of heart, this won't be merged here. So let's do what you said; "StaticBitSets.jl" sounds like a fine name. Can you open the repo and set me as maintainer? |
Done, you should have an invitation and you should be an admin. Sorry I was so slow at getting around to that 😬 |
|
Good point about the difficulties with It looks like the code is in pretty solid shape here, so I hope it's a rather simple matter of copy & paste to turn it into a shiny new package! |
|
Since that other repo is no longer active, what could be done to advance the proposal from here? @c42f |
6 participants
This PR adds static bitsets. The last attempt #552 got somewhat out of sync with what I'm using, so I rewrote the PR.
The code is currently very separate from the rest of StaticArrays (single self-contained new feature). As discussed in the now abandoned PR, it does not make a lot of sense to adopt either of the AbstractArray or AbstractSet interface. That's OK, we don't need to follow a Base interface.
I see possible need for bikeshedding with the SBitSet / MBitSet promotion behavior, as well as broadcasting. My thinking was that broadcasting with loop-fusion is probably not needed, since typical sizes will be fully unrolled (i.e. there is no loop) and possibly even fit into a big vector register.
The promotion behavior in this PR is "everything becomes SBitSet", which is markedly different from StaticArray's "everything becomes MArray". This behavior made more sense for SBitSet in my mind. If you disagree, then please explain.
Further missing are some docs and examples, and I'll need to do a little refactor with respect to uniform coding style (e.g. import instead of using fully qualified names for method extension).