LUCENE-9142 Refactor IntSet operations for determinize #1184

madrob · 2020-01-18T00:21:59Z

Fix a bug where a frozen set could be symmetrically unequal to the
sorted set that created it because we compared the backing array
instead of only the active elements.

lucene/core/src/java/org/apache/lucene/util/automaton/FrozenIntSet.java

lucene/core/src/java/org/apache/lucene/util/automaton/IntSet.java

lucene/core/src/java/org/apache/lucene/util/automaton/SortedIntSet.java

mikemccand

Thank you for tackling this @madrob!

lucene/core/src/java/org/apache/lucene/util/automaton/FrozenIntSet.java

lucene/core/src/java/org/apache/lucene/util/automaton/IntSet.java

mikemccand · 2020-01-21T16:29:25Z

lucene/core/src/java/org/apache/lucene/util/automaton/SortedIntSet.java

@@ -77,6 +88,7 @@ public void incr(int num) {
        values[i] = num;
        counts[i] = 1;
        upto++;
+        stale = true;


How come the code didn't need/use stale before?

Because it didn't track whether the cached hash code was stale and relied on the caller to do it. I feel like it is more clear to manage it internally.

oh, found the replacement we should use: ArrayUtil.copyOfSubArray

mikemccand · 2020-01-21T16:29:52Z

lucene/core/src/java/org/apache/lucene/util/automaton/SortedIntSet.java

  int[] counts;
  int upto;
-  private int hashCode;
+
+  // Tracks if the hashCode computation is out of date and also if the array is out of sync with the map


Isn't the array discarded as soon as we switch over to the map (so how could it be out of sync)?

The array is used for equality comparison.

lucene/core/src/java/org/apache/lucene/util/automaton/SortedIntSet.java

mikemccand · 2020-01-21T16:31:41Z

lucene/core/src/test/org/apache/lucene/util/automaton/TestIntSet.java

+
+@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
+public class TestIntSet {
+    @Test


We don't need @Test annotations -- LuceneTestCase runner knows test* methods are tests.

I think you can remove these @Test annotations?

mikemccand · 2020-01-21T16:35:48Z

lucene/core/src/java/org/apache/lucene/util/automaton/SortedIntSet.java


  // If we hold more than this many states, we switch from
  // O(N^2) linear ops to O(N log(N)) TreeMap
-  private final static int TREE_MAP_CUTOVER = 30;
+  private final static int TREE_MAP_CUTOVER = 32;

  private final Map<Integer,Integer> map = new TreeMap<>();


Would be nice to use a native map from HPPC instead -- does HPPC have a sorted map? Hmm, but this is core, and we don't have any dependencies in Lucene's core :)

mikemccand · 2020-01-21T16:37:21Z

Also, I suspect that branch of code that was (asymmetrically) comparing IntSet and FrozenIntSet where it was using values.length instead of upto was actually dead code. Maybe we can remove that comparison path?

madrob

One there consideration is whether we need to do the array/map split at all. For small cases where the map overhead may be comparatively large, the total case may be small enough that it doesn't matter. I haven't done performance profiling here to determine this, but note that once we switch to map we don't go back to arrays until we get down to zero, not if we dip back below the threshold.

Split SortedIntSet into a class heirarchy to make comparisons to FrozenIntSet more meaningful. Use Arrays.equals for more efficient comparison.

madrob · 2020-01-23T19:07:24Z

Started over on this, tried to be less ambitious but still improving the situation I think.

~~Unclear why precommit failed according to GitHub, when I click through it looks like it succeeds?~~
Needed to refresh for this.

@mikemccand @dweiss @uschindler can you take another look?

mikemccand

Changes look great! So you solved the crazy asymmetric equals bug from before by fixing the inheritance so now it's always IntSet.equals that runs, nice.

I wonder if any of the luceneutil queries would exercise (benchmark) these classes? I don't think so? Who are the heavy consumers of determinize?

lucene/core/src/java/org/apache/lucene/util/automaton/FrozenIntSet.java

mikemccand · 2020-02-04T14:10:25Z

lucene/core/src/test/org/apache/lucene/util/automaton/TestIntSet.java

+
+@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
+public class TestIntSet {
+    @Test


I think you can remove these @Test annotations?

madrob · 2020-02-04T20:25:54Z

Who are the heavy consumers of determinize?

Fuzzy queries using multi-byte code points. Spell check (I think again, with multi-byte points). Possibly some of the graph queries, but I don't completely understand the context of how those build up their automata.

* LUCENE-9142 Refactor SortedIntSet for equality Split SortedIntSet into a class heirarchy to make comparisons to FrozenIntSet more meaningful. Use Arrays.equals for more efficient comparison. Add tests for IntSet to verify correctness.

* LUCENE-9142 Refactor IntSet operations for determinize (#1184) Co-authored-by: Mike <madrob@users.noreply.github.com>

…ld not clone bitsets repeatedly (apache#1184) Co-authored-by: David Smiley <dsmiley@salesforce.com>

…ld not clone bitsets repeatedly (#1184) (#2675) Co-authored-by: David Smiley <dsmiley@salesforce.com>

dweiss reviewed Jan 18, 2020

View reviewed changes

mikemccand reviewed Jan 21, 2020

View reviewed changes

madrob commented Jan 21, 2020

View reviewed changes

LUCENE-9142 Refactor SortedIntSet for equality

e9d8313

Split SortedIntSet into a class heirarchy to make comparisons to FrozenIntSet more meaningful. Use Arrays.equals for more efficient comparison.

madrob force-pushed the lucene9142 branch from 3997ea0 to e9d8313 Compare January 23, 2020 16:57

madrob requested review from dweiss and uschindler January 23, 2020 16:57

LUCENE-9142 Add tests for IntSet

0fea83e

mikemccand approved these changes Feb 4, 2020

View reviewed changes

LUCENE-9142 Review feedback

1675ea7

madrob merged commit abd282d into apache:master Feb 6, 2020

madrob deleted the lucene9142 branch February 6, 2020 20:42

mikemccand pushed a commit that referenced this pull request Jun 23, 2021

Backport LUCENE-9142 and LUCENE-9983 (#2517)

c6b9dd9

* LUCENE-9142 Refactor IntSet operations for determinize (#1184) Co-authored-by: Mike <madrob@users.noreply.github.com>

risdenk added a commit to risdenk/lucene-solr that referenced this pull request Nov 22, 2022

SOLR-16555: SolrIndexSearcher - FilterCache intersections/andNot shou…

5841191

…ld not clone bitsets repeatedly (apache#1184) Co-authored-by: David Smiley <dsmiley@salesforce.com>

risdenk added a commit to risdenk/lucene-solr that referenced this pull request Nov 22, 2022

SOLR-16555: SolrIndexSearcher - FilterCache intersections/andNot shou…

dfa2c1f

…ld not clone bitsets repeatedly (apache#1184) Co-authored-by: David Smiley <dsmiley@salesforce.com>

risdenk added a commit that referenced this pull request Nov 28, 2022

SOLR-16555: SolrIndexSearcher - FilterCache intersections/andNot shou…

1a96480

…ld not clone bitsets repeatedly (#1184) (#2675) Co-authored-by: David Smiley <dsmiley@salesforce.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LUCENE-9142 Refactor IntSet operations for determinize #1184

LUCENE-9142 Refactor IntSet operations for determinize #1184

madrob commented Jan 18, 2020

mikemccand left a comment

mikemccand Jan 21, 2020

madrob Jan 21, 2020

dweiss Jan 22, 2020

mikemccand Jan 21, 2020

madrob Jan 21, 2020

mikemccand Jan 21, 2020

mikemccand Feb 4, 2020

mikemccand Jan 21, 2020

mikemccand commented Jan 21, 2020

madrob left a comment

madrob commented Jan 23, 2020 •

edited

mikemccand left a comment

mikemccand Feb 4, 2020

madrob commented Feb 4, 2020

LUCENE-9142 Refactor IntSet operations for determinize #1184

LUCENE-9142 Refactor IntSet operations for determinize #1184

Conversation

madrob commented Jan 18, 2020

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemccand commented Jan 21, 2020

madrob left a comment

Choose a reason for hiding this comment

madrob commented Jan 23, 2020 • edited

mikemccand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madrob commented Feb 4, 2020

madrob commented Jan 23, 2020 •

edited