Skip to content
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.

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

Add intersect_vector16_inplace to enable vectorized inplace intersection between arrays. #445

Merged

Conversation

CharlesChen888
Copy link
Contributor

@CharlesChen888 CharlesChen888 commented Mar 10, 2023

I notice that in function array_container_intersection_inplace, there is no implementation of vectorized inplace intersection, and this slows down the computation.

We can simply use intersect_vector16 to do a non-inplace intersection, and replace the source array with the new one, but this requires frequent memory allocation and freeing. So I think a real inplace intersection is better.

Function intersect_vector16_inplace is modified from intersect_vector16, which uses __m128i tmp[2] for temporary storage of intersection result, to avoid overwritting original numbers in source array that haven't been used.

@lemire
Copy link
Member

lemire commented Mar 10, 2023

Running tests. Looks good at a glance!!!

} else if (b < a) {
i_b++;
} else {
A[count] = a; //==b;

Check notice

Code scanning / CodeQL

Commented-out code

This comment appears to contain commented-out code.
Copy link
Member

Choose a reason for hiding this comment

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

GitHub complains about commented out code, make sure that you don't need to delete it.

@CharlesChen888
Copy link
Contributor Author

I found this in fuzz/croaring_fuzzer.c. Looks like we need to comment out the first line.

int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size){
int LLVMFuzzerTestOneInput(const char *data, size_t size) {
    // We test that deserialization never fails.
    roaring_bitmap_t* bitmap = roaring_bitmap_portable_deserialize_safe(data, size);
    if(bitmap) {
        // The bitmap may not be usable if it does not follow the specification.
        roaring_bitmap_free(bitmap);
    }
    return 0;
}

@CharlesChen888
Copy link
Contributor Author

@lemire I have rebased this branch to the latest commit, please rerun the tests.

@lemire
Copy link
Member

lemire commented Mar 14, 2023

I found this in fuzz/croaring_fuzzer.c. Looks like we need to comment out the first line.

This was a typo, I fixed it earlier.

@lemire
Copy link
Member

lemire commented Mar 14, 2023

Running tests.

@CharlesChen888
Copy link
Contributor Author

@lemire Seems that fuzzers won't build. Have you got any idea why? Or does it matter?

@lemire
Copy link
Member

lemire commented Mar 14, 2023

This is a good PR. I am merging it. I will figure out the issue with the fuzzer later. It is unrelated to your code, I expect.

@lemire lemire merged commit 84fe3c8 into RoaringBitmap:master Mar 14, 2023
@feng-y feng-y mentioned this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants