WIP: Refactor IntSets to use BitVectors #10065

mbauman · 2015-02-04T00:33:53Z

This WIP is to merge my IntSets.jl project into base. Here's what this entails:

Use BitVectors instead of a mismash of bitvector.c and ad-hoc Julia code.
IntSet is much better defined to only hold integers in 0:typemax(Int)-1. Before it wasn't consistently defined, particularly when it came to complement IntSets.
Invert the complement IntSet semantics. Instead of filling ones outside the range of the IntSet, simply reverse what a set bit means. This means that inverting an IntSet is O(1) instead of O(n) and fixes a few bugs.
For mathematical functions (intersect, union, etc.), use BitVector functions to work 64 bits at a time instead of working bit-by-bit.
Increase test-coverage to 100%
Would close IntSet should be a wrapper around a BitArray #3039 (with a new constructor)

~~There are still two outstanding issues here that I could use some help with:~~ The only thing left is a performance regression:

Julia fails to launch with a segfault when the sysimg.{dll/dylib/so} is unavailable. It happens during the compilation of an inference function that makes heavy use of IntSets, but I can't tell why with my rudimentary LLDB/LLVM knowledge. My hunch is that I've missed some functions that are needed but load after inference.jl in the sysimg, but it successfully bootstraps… so I'm not sure. The new inference bootstrap separation made this a cinch to debug. The culprits were some redundant Base. module-qualified names (since Base doesn't exist yet).
While many functions are way faster, there's still a performance gap for a few functions; most notably ~2x for in. See these albums for a comparison against the current master with varying density and size. Some of this is due to the addition of an unnecessary GC frame (propagate effect_free information out of functions #9974), but I think most of it is from the extra dereference and perhaps some more-frequent cache misses. Particularly confusing to me is in — there are fewer branches and the LLVM/native code looks much better (especially once I manually inline the getindex call to eliminate the GC frame), but it still lags in performance. That said, it doesn't seem to have an effect on overall compilation times from looking at the linalg test suite.

JeffBezanson · 2015-02-04T00:57:34Z

Very nice. The thorough performance comparison is pretty impressive!

tkelman · 2015-02-04T01:01:11Z

Remember to add intset to test/runtests.jl

mbauman · 2015-04-16T22:25:10Z

Interesting thought from #1032: If we can somehow manage to remove support for 0 in IntSet, we could almost implement find(::BitArray) as a very fast and lazy conversion to an IntSet.

I'm not sure how practical that would be since IntSet doesn't implement indexing (and cannot do so efficiently). And it wouldn't satisfy the current meaning of find because of that. But if we're clever, it might simplify and improve the performance of logical indexing.

quinnj · 2015-04-16T22:28:53Z

Maybe we just create a new type IndexSet that has those behaviors?

StefanKarpinski · 2015-04-17T14:15:05Z

IndexSet is really a better name for this type anyway.

* Use BitVectors instead of a mismash of bitvector.c and ad-hoc Julia code. * Invert the complement IntSet semantics. Instead of filling ones outside the range of the IntSet, simply reverse what a set bit means. * For mathematical functions (intersect, union, etc.), use BitVector functions to work 64 bits at a time instead of working bit-by-bit. * Increase test-coverage to 100%

mbauman · 2015-05-21T00:18:54Z

Bump! With inference decoupled from base, this became very easy to debug! All tests pass now. The only remaining issue is the performance gap on in.

dpsanders · 2015-07-21T14:55:05Z

Bump... I spent time yesterday writing tests for IntSets, only to discover that @mbauman had already done it and completely rewritten everything! +1! (Maybe you can separate out the tests and get those merged first.)

mbauman · 2015-07-21T15:23:52Z

Shucks. Sorry for the duplicated effort. Don't let this long-languishing work in progress hold up your testing work! Feel free to push your own tests and/or grab tests from here.

If you do grab tests from here, you'll run into some complement IntSet bugs. I have a hunch that nobody is using complement IntSets very thoroughly.

Maybe we should deprecate the complement functionality? This is a core piece of functionality that's required for inference (otherwise I don't think it'd be in base). It could make sense to simplify IntSet and move this work into a package for richer IndexSet-like functionality.

JeffBezanson · 2015-07-21T15:26:27Z

+1 for removing the complement feature. It was written back in the days when we were bored and just looking for neat stuff to do.

StefanKarpinski · 2015-07-21T15:29:30Z

Agree with removing the complement stuff. I also suspect this type should be renamed to IndexSet since it is really not a generic IntSet but really a set of indices.

dpsanders · 2015-07-21T15:44:03Z

My effort is at #12247.

The complement functionality is neat, but I can see that it doesn't really belong in Base.
My tests are quite focused around the particular current implementation.

ScottPJones · 2015-07-21T15:49:57Z

I'd agree if this only supports 0 and above and is really only used for indices it should just be added as IndexSet, with no complement. IntSet to me implies it needs to allow any integer value in the set, not just >= 0.

mbauman · 2015-07-21T15:58:19Z

Perhaps we entirely deprecate IntSet and introduce IndexSet? That would allow us to gracefully remove the ability to store zero, too.

ScottPJones · 2015-07-21T16:01:33Z

SGTM 👍

StefanKarpinski · 2015-07-21T16:03:41Z

I'm entirely convinced that we should remove the ability to store zero...

mbauman · 2015-07-21T16:06:14Z

Wonderful. Let's do it. It's actually not as bad as I had been thinking since we already branch for a bounds check. We can check for zero within that branch with no extra overhead to the other branch.

pabloferz · 2015-08-05T18:04:24Z

base/intset.jl

    else
-        (s.bits[n>>5 + 1] & (UInt32(1)<<(n&31))) != 0
+        ifelse((idx <= 0) | (idx > typemax(Int)), false, s.inverse)
    end
 end


Aren't you missing the return statements here?

That would probably be more clear, yes, but this works just fine as it is.

The result of the last expression in a function body is used as the return value (if not explicitly specified). In this case, the last expression is the if block.

The result of an if block is similarly determined by the last expression of the traversed branch (or nothing if no branches were traversed).

Ok, I knew that the return value of the last expression was used, but I though that an if block would have returned nothing.

Sorry for the noise.

mbauman · 2015-08-09T20:41:46Z

Closing in favor of JuliaCollections/DataStructures.jl#114 (moving IntSet to DataStructures) and #12270 (deprecating complement and stored zeros, on the path to renaming IntSet → IndexSet).

* Complete deprecation of stored zeros; IntSets now only support integers in the range `1:typemax(Int)` * Complete deprecation of `complement`; removes all support for inverted IntSets * Refactor internals to rely on a BitVector, allowing the use of highly optimized `map` methods. `IntSet` is now immutable. This significantly improves performance across varying [densities](http://imgur.com/a/uqv8A) and [sizes](http://imgur.com/a/iEgcr). These are compared against a modified Base with deprecation warnings removed for a fairer comparison. Testing code [available here](https://github.com/mbauman/IntSets.jl/tree/b50a7c97abbe9786e33221f723e107e266f31fe4/test). * Add more tests and organize into testsets. * Improve hashing; `hash(IntSet([1]))` is now distinct from `hash(IntSet([65]))` This is a continuation of #10065. Now that complements are fully removed, making IntSet immutable solves the performance issue. I am keeping the name the same within this PR as it vastly simplifies comparisons between the two implementations; the name can later be changed to `IndexSet` if still desired. The naming story is now a bit more complicated since we support offset indices, but a future change could perhaps allow wrapping any `AbstractVector{Bool}` and base the supported `Int`s on those indices. Very few methods depend upon BitArray internals.

mbauman force-pushed the mb/intsetrefactor branch from 5bc5969 to 782f34c Compare March 10, 2015 15:04

mbauman mentioned this pull request Apr 17, 2015

Change for i=1:length(A) to for i in eachindex(A) #10858

Merged

6 tasks

mbauman force-pushed the mb/intsetrefactor branch 2 times, most recently from 304d113 to 5a53338 Compare May 20, 2015 19:42

mbauman added 3 commits May 20, 2015 15:47

Load some BitArray machinery sooner during bootstrap

49e881d

Remove superfluous Base module lookups

204d5f2

mbauman force-pushed the mb/intsetrefactor branch from 5a53338 to a4f7a73 Compare May 20, 2015 19:47

carlobaldassi mentioned this pull request May 22, 2015

Speed up scalar BitArray indexing by ~25% #11403

Closed

hayd mentioned this pull request May 22, 2015

WIP: try ordered Dict representation #10116

Closed

mbauman mentioned this pull request Jul 22, 2015

Deprecate IntSet complement and stored zeros #12270

Merged

pabloferz reviewed Aug 5, 2015
View reviewed changes

mbauman mentioned this pull request Aug 9, 2015

Add IntSet to DataStructures JuliaCollections/DataStructures.jl#114

Closed

mbauman closed this Aug 9, 2015

kmsquire mentioned this pull request Jan 14, 2016

Tag DataStructures v0.4.2 JuliaLang/METADATA.jl#4412

Merged

kmsquire mentioned this pull request Jul 27, 2016

Tests fail on v0.5 JuliaCollections/DataStructures.jl#209

Closed

mbauman mentioned this pull request Feb 4, 2017

Refactor IntSets #20456

Merged

mbauman deleted the mb/intsetrefactor branch January 11, 2021 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Refactor IntSets to use BitVectors #10065

WIP: Refactor IntSets to use BitVectors #10065

mbauman commented Feb 4, 2015

JeffBezanson commented Feb 4, 2015

tkelman commented Feb 4, 2015

mbauman commented Apr 16, 2015

quinnj commented Apr 16, 2015

StefanKarpinski commented Apr 17, 2015

mbauman commented May 21, 2015

dpsanders commented Jul 21, 2015

mbauman commented Jul 21, 2015

JeffBezanson commented Jul 21, 2015

StefanKarpinski commented Jul 21, 2015

dpsanders commented Jul 21, 2015

ScottPJones commented Jul 21, 2015

mbauman commented Jul 21, 2015

ScottPJones commented Jul 21, 2015

StefanKarpinski commented Jul 21, 2015

mbauman commented Jul 21, 2015

pabloferz Aug 5, 2015

mbauman Aug 5, 2015

pabloferz Aug 5, 2015

mbauman commented Aug 9, 2015

WIP: Refactor IntSets to use BitVectors #10065

WIP: Refactor IntSets to use BitVectors #10065

Conversation

mbauman commented Feb 4, 2015

JeffBezanson commented Feb 4, 2015

tkelman commented Feb 4, 2015

mbauman commented Apr 16, 2015

quinnj commented Apr 16, 2015

StefanKarpinski commented Apr 17, 2015

mbauman commented May 21, 2015

dpsanders commented Jul 21, 2015

mbauman commented Jul 21, 2015

JeffBezanson commented Jul 21, 2015

StefanKarpinski commented Jul 21, 2015

dpsanders commented Jul 21, 2015

ScottPJones commented Jul 21, 2015

mbauman commented Jul 21, 2015

ScottPJones commented Jul 21, 2015

StefanKarpinski commented Jul 21, 2015

mbauman commented Jul 21, 2015

pabloferz Aug 5, 2015

Choose a reason for hiding this comment

mbauman Aug 5, 2015

Choose a reason for hiding this comment

pabloferz Aug 5, 2015

Choose a reason for hiding this comment

mbauman commented Aug 9, 2015