Speed up NumericDocValuesWriter with index sorting

[easyice]

Description

like pr-399, the DocsWithFieldSet#add() can avoid create instance for FixedBitSet in dense scene, so in NumericDocValuesWriter#sortDocValues() we can do the same thing, just the way to judge dense or sparse, I'm not sure if it's rigorous enough

the benchmark for write ten SortedNumericDocValuesField, the optimization saves ~7% commit time

public class IndexBenchMarksNDV {

  public static void main(final String[] args) throws Exception {
    doWriteNDV();
  }

  static void doWriteNDV() throws IOException {
    BenchMark benchMark = new BenchMark(5, 5, 1000000);
    benchMark.run();
  }

  static class BenchMark {
    final int warmup;
    final int numValues;
    final int loopCount;

    Directory dir;
    IndexWriter indexWriter;

    BenchMark(int warmup, int loopCount, int numValues) {
      this.warmup = warmup;
      this.numValues = numValues;
      this.loopCount = loopCount;
    }

    private void init() throws IOException {
      Path tempDir = Files.createTempDirectory(Paths.get("/Volumes/RamDisk"), "tmp");
      dir = MMapDirectory.open(tempDir);
      IndexWriterConfig iwc = new IndexWriterConfig(null);
      iwc.setMergePolicy(NoMergePolicy.INSTANCE);
      iwc.setMaxBufferedDocs(IndexWriterConfig.DISABLE_AUTO_FLUSH);
      Sort indexSort = new Sort(new SortedNumericSortField("f1", SortField.Type.LONG));
      iwc.setIndexSort(indexSort);
      indexWriter = new IndexWriter(dir, iwc);
    }

    private void close() throws IOException {
      indexWriter.close();
      dir.close();
    }

    private long doWrite() throws IOException {
      for (int i = 0; i < numValues; i++) {
        Document document = new Document();
        for (int f = 0; f < 10; f++) {
          document.add(new SortedNumericDocValuesField("f" + f, i / 1000));
        }
        indexWriter.addDocument(document);
      }
      Document document = new Document();
      for (int f = 0; f < 10; f++) {
        document.add(new SortedNumericDocValuesField("f" + f, 1));
      }
      indexWriter.addDocument(document);
      long t0 = System.nanoTime();
      indexWriter.commit();
      return System.nanoTime() - t0;
    }

    void run() throws IOException {
      init();
      for (int i = 0; i < warmup; i++) {
        doWrite();
      }
      System.gc();
      List<Double> times = new ArrayList<>();
      for (int i = 0; i < loopCount; i++) {
        long took = doWrite();
        times.add(took / 1000000D);
      }
      double min = times.stream().mapToDouble(Number::doubleValue).min().getAsDouble();
      System.out.println("took(ms):" + String.format(Locale.ROOT, "%.2f", min));
      close();
    }
  }
}

[jpountz]

I left some comments about the implementation but it looks like a good optimization to me. Can you also add a CHANGES entry under 9.8?

[jpountz]

I would rather like to expose something like boolean dense() instead of the internal bitset.

[easyice]

ok, it is fixed

[jpountz]

We currently only have 2 implementations of BitSet, which the JVM optimizes better than N implementations. Could we remove this special BitSet implementation and use a special null marker instead to imply that all docs match?

[easyice]

great suggestion! i tested earlier the virtual function can be inline always if there are only 2 implementations

[easyice]

@jpountz Thank you for comments, it's very helpful to me, the code has updated.

[jpountz]

I left some minor comments, it looks good to me otherwise.

[jpountz]

Only testing dense() should be correct right?

Suggested change

	docsWithField.dense() && sortMap.size() == docsWithField.cardinality());
	docsWithField.dense());

[easyice]

no,DocsWithFieldSet will update it's bitset when a doc with field really added only, in sparse, it will not call DocsWithFieldSet#add, so if the first 64 doc with field has added, and then some doc added without this filed, the docsWithField.dense() will return true, i think we can remove docsWithField.dense(), use sortMap.size() == docsWithField.cardinality() only for dense case, the sortMap.size() will return the number of documents for the LeafReader, what do you think?

[jpountz]

you're right! thanks for explaining

[easyice]

i have removed docsWithField.dense()

[jpountz]

and likewise here?

Suggested change

	docsWithField.dense() && sortMap.size() == docsWithField.cardinality());
	docsWithField.dense());

[jpountz]

Looking at the call site, this is only called when target is less than maxDoc, so thit could be simplified to just return target without the target < maxDoc check?

[easyice]

Nice catch, it's updated