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
wants to merge 11 commits into from
Closed

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

Changes from 3 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
9 changes: 8 additions & 1 deletion 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 @@ -29,6 +29,7 @@
public final class BitSetMethods {

private static final long WORD_SIZE = 8;
private static final int SIZE_OF_LONG = Long.SIZE;

private BitSetMethods() {
// Make the default constructor private, since this only holds static methods.
Expand Down Expand Up @@ -71,7 +72,13 @@ 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) {
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.

Here, bitSetWidthInBytes should be changed to bitSetWidthInWords and the rest of the code should be updated accordingly.

for (int i = 0; i <= bitSetWidthInBytes; i++) {
long widthInLong = bitSetWidthInBytes / SIZE_OF_LONG;
Copy link
Contributor

@rxin rxin May 5, 2015

Choose a reason for hiding this comment

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

width needs to be an int too :)

for (int i = 0; i <= widthInLong; i++) {
if (PlatformDependent.UNSAFE.getLong(baseObject, baseOffset + i) != 0) {
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 don't think that this is correct, since i loops over words but the second argument to getLong is measured in bytes. You could do i * 8 instead.

Reynold pointed out that there might be faster micro-optimizations tricks that can be played here; see https://groups.google.com/d/msg/mechanical-sympathy/k0qd7dLHFQE/gsTFSjP6MVMJ

return true;
}
}
for (int i = (int)(SIZE_OF_LONG * widthInLong); i < bitSetWidthInBytes; i++) {
Copy link
Contributor

@JoshRosen JoshRosen May 5, 2015

Choose a reason for hiding this comment

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

It looks like this block now contains two for loops. A bad merge conflict resolution, perhaps?

Copy link
Contributor Author

@tedyu tedyu May 5, 2015

Choose a reason for hiding this comment

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

I don't think so.
The second loop is for the remaining bytes :-)

Copy link
Contributor

@JoshRosen JoshRosen May 5, 2015

Choose a reason for hiding this comment

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

Ah, sorry for overlooking that. I guess I was thinking of optimizing for the case where the bitset width was a multiple of the word size.

Copy link
Contributor Author

@tedyu tedyu May 5, 2015

Choose a reason for hiding this comment

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

That case is covered by the first loop.

Copy link
Contributor

@rxin rxin May 5, 2015

Choose a reason for hiding this comment

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

do we need the 2nd loop? I think the assumption is that all bitsets are word-aligned. We should definitely document that though.

if (PlatformDependent.UNSAFE.getByte(baseObject, baseOffset + i) != 0) {
return true;
}
Expand Down