Skip to content

SPARK-7450 Use UNSAFE.getLong() to speed up BitSetMethods#anySet() #5897

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.

Sign up for GitHub

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

Closed
tedyu wants to merge 11 commits into from
Closed

SPARK-7450 Use UNSAFE.getLong() to speed up BitSetMethods#anySet() #5897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3e9b691
Use UNSAFE.getLong() to speed up BitSetMethods#anySet()
tedyu May 4, 2015
4ca0ef6
Use UNSAFE.getLong() to speed up BitSetMethods#anySet()
tedyu May 5, 2015
63ee050
Use UNSAFE.getLong() to speed up BitSetMethods#anySet()
tedyu May 5, 2015
093b7a4
Use UNSAFE.getLong() to speed up BitSetMethods#anySet()
tedyu May 5, 2015
855374b
Remove second loop since bitSetWidthInBytes is WORD aligned
tedyu May 5, 2015
75a467b
Correct offset for UNSAFE.getLong()
tedyu May 6, 2015
817e3f9
Replace constant 8 with SIZE_OF_LONG
tedyu May 6, 2015
83f9f87
Merge branch 'master' of github.com:apache/spark
tedyu May 6, 2015
b51dcaf
Add unit test in BitSetSuite for BitSet#anySet()
tedyu May 6, 2015
1719c5b
Correct upper bound in for loop
tedyu May 6, 2015
473bf9d
Address Josh's review comments
tedyu May 6, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions unsafe/src/main/java/org/apache/spark/unsafe/bitset/BitSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,4 +102,12 @@ public boolean isSet(int index) {
public int nextSetBit(int fromIndex) {
return BitSetMethods.nextSetBit(baseObject, baseOffset, fromIndex, numWords);
}

/*
* @return whether any bit in the BitSet is set
*/
public boolean anySet() {
Copy link
Contributor

@JoshRosen JoshRosen May 7, 2015

Choose a reason for hiding this comment

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

Should match BitSetMethods's description:

/**
    * Returns {@code true} if any bit is set.
    */

return BitSetMethods.anySet(baseObject, baseOffset, numWords);
}

}
9 changes: 5 additions & 4 deletions unsafe/src/main/java/org/apache/spark/unsafe/bitset/BitSetMethods.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
*/
public final class BitSetMethods {

private static final long WORD_SIZE = 8;
static final long WORD_SIZE = 8;
Copy link
Contributor

@JoshRosen JoshRosen May 7, 2015

Choose a reason for hiding this comment

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

Could remain private.


private BitSetMethods() {
// Make the default constructor private, since this only holds static methods.
Expand Down Expand Up @@ -70,9 +70,10 @@ public static boolean isSet(Object baseObject, long baseOffset, int index) {
/**
* Returns {@code true} if any bit is set.
*/
public static boolean anySet(Object baseObject, long baseOffset, long bitSetWidthInBytes) {
for (int i = 0; i <= bitSetWidthInBytes; i++) {
if (PlatformDependent.UNSAFE.getByte(baseObject, baseOffset + i) != 0) {
public static boolean anySet(Object baseObject, long baseOffset, long bitSetWidthInWords) {
long addr = baseOffset;
for (int i = 0; i < bitSetWidthInWords; i++, addr += WORD_SIZE) {
if (PlatformDependent.UNSAFE.getLong(baseObject, addr) != 0) {
return true;
}
}
Expand Down
7 changes: 7 additions & 0 deletions unsafe/src/test/java/org/apache/spark/unsafe/bitset/BitSetSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public void basicOps() {
for (int i = 0; i < bs.capacity(); i++) {
Assert.assertFalse(bs.isSet(i));
}
// another form of asserting that the bit set is empty
Assert.assertFalse(bs.anySet());
Copy link
Contributor

@JoshRosen JoshRosen May 6, 2015

Choose a reason for hiding this comment

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

I still think that we should write a proper unit test for anySet that will create a new bitset spanning multiple words, then set a single in each word, then check that anySet() returns true. This test isn't sufficient because it wouldn't have caught any of the bugs in earlier iterations of this PR.


// Set every bit and check it.
for (int i = 0; i < bs.capacity(); i++) {
Expand All @@ -52,6 +54,11 @@ public void basicOps() {
bs.unset(i);
Assert.assertFalse(bs.isSet(i));
}

// Make sure anySet() can detect any set bit
bs = createBitSet(256);
bs.set(64);
Assert.assertTrue(bs.anySet());
}

@Test
Expand Down