Short-circuiting AndFilter. #3676
Conversation
|
👍 |
|
Thanks @gianm What do the benchmark numbers look like? |
|
I have some review comments but it's not letting me publish them on mobile. Give me a few mins. |
| } | ||
|
|
||
| @Test | ||
| public void testAndSelector() |
There was a problem hiding this comment.
Can you add NOT(AND(...)) cases here?
I'm specifically curious if the new allFalse filter wrapping has any corner cases under a NOT outer filter.
There was a problem hiding this comment.
Filters.allFalse just returns an empty bitmap, which is the same thing that AND would have done before the short-circuiting, so there should be no change in behavior there.
Added tests anyway though.
| List<ImmutableBitmap> bitmaps = Lists.newArrayList(); | ||
| for (int i = 0; i < filters.size(); i++) { | ||
| bitmaps.add(filters.get(i).getBitmapIndex(selector)); | ||
| final List<ImmutableBitmap> bitmaps = Lists.newArrayList(); |
There was a problem hiding this comment.
Can you go ahead and give the list capacity for filters.size at construction?
gianm
force-pushed
the
and-short-circuit
branch
from
e36d7fa
to
f4afc23
Compare
|
@drcrallen I pushed changes for your two line comments. I don't think it's worth writing a benchmark though, since it seems clear that short circuiting is good and that in the normal case there won't be a penalty (nothing really different is happening). |
| List<ImmutableBitmap> bitmaps = Lists.newArrayList(); | ||
| for (int i = 0; i < filters.size(); i++) { | ||
| bitmaps.add(filters.get(i).getBitmapIndex(selector)); | ||
| final List<ImmutableBitmap> bitmaps = Lists.newArrayListWithCapacity(filters.size()); |
There was a problem hiding this comment.
This is more noisy than new ArrayList<>(filters.size()). Lists.newArrayList(), Sets.newHashSet(), etc. are considered obsolete since Java 7: http://stackoverflow.com/a/9980949/648955
I suggest to standardise on new ArrayList<>() form and prohibit Lists.newArrayList() using checkstyle: http://checkstyle.sourceforge.net/config_regexp.html
There was a problem hiding this comment.
I dunno, I think the Lists methods have nicer names and overloads.
List<Integer> xs = Lists.newArrayList(filters.size()); // a List<Integer> with one element, filters.size()
List<Filter> xs = Lists.newArrayList(filters); // a List<Filter> that is a copy of "filters"
List<Filter> xs = Lists.newArrayListWithCapacity(filters.size()); // a List<Filter> with filters.size() elementsseems pretty nice to me. Especially since the first doesn't have a jdk equivalent, and the second's jdk equivalent only works on Collections, whereas guava works on Iterables and Iterators too.
There was a problem hiding this comment.
@gianm well, I'm against no-arg Lists.newArrayList() and Lists.newArrayListWithCapacity() because I think everybody knows the meaning of the ArrayList(int) constructor parameter.
Of cause not insisting on both, it's just my opinion. If there is no general agreement let's don't change anything, this is not a big deal. I will stop mentioning this during reviews.
| offset = new BitmapOffset( | ||
| selector.getBitmapFactory(), | ||
| selector.getBitmapFactory().intersection(bitmaps), | ||
| new AndFilter(preFilters).getBitmapIndex(selector), |
There was a problem hiding this comment.
Maybe move the filter intersection logic into a static method (belonging to AndFilter, for example, and call it from here and from inside AndFilter. getBitmapIndex()? Because creating a dandling instance looks confusing (somebody forgot to create a variable?) to people and to static analysis tools.
There was a problem hiding this comment.
Ok, moved that to a static.
gianm
force-pushed
the
and-short-circuit
branch
from
f4afc23
to
e132f27
Compare
If any of the bitmaps are empty, the result will be false.
If any of the bitmaps are empty, the result will be false.
Fixes #3675.