You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "easyice (via GitHub)" <gi...@apache.org> on 2023/06/20 06:49:28 UTC
[GitHub] [lucene] easyice opened a new pull request, #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
easyice opened a new pull request, #12377:
URL: https://github.com/apache/lucene/pull/12377
### Description
This small change will reduce an unnecessary loop in DirectMonotonicWriter#flush
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jpountz commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1600386083
Great, thanks for checking!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand merged pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand merged PR #12377:
URL: https://github.com/apache/lucene/pull/12377
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1599201450
> Have we checked if this actually made things faster? I remember getting surprised in the past because folding too much into a single loop would prevent C2 from recognizing that some bits can be auto-vectorized (like computing the min value across the entire array).
Hmm, tricky. I don't think we've tested if it's actually faster. We could wait for nightlies to see any impact? Or, revert now and benchmark before pushing again? Darned fragile auto-vectorization... if these loops are an example of that, let's at least add a comment explaining so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1601234854
Though, separately, I wonder why this code does not also use the "best/simple linear fit" compression too ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gf2121 commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "gf2121 (via GitHub)" <gi...@apache.org>.
gf2121 commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1598272181
LGTM, Thanks @easyice !
Could you please add a CHANGES entry under 9.8.0 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1601232189
Duh, I think we cannot actually merge those two loops? Seems we must first make a pass to find the min across all values, before subtracting it from all values..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1598760981
Thank you @easyice -- I merged this and backported to `branch_9x` as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] easyice commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "easyice (via GitHub)" <gi...@apache.org>.
easyice commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1601914251
> Though, separately, I wonder why this code does not also use the "best/simple linear fit" compression too ...
Yes, the loop in pack really cannot be merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1600798058
@jpountz observed that `OrdinalMap` uses a similar idea but different code path: https://github.com/apache/lucene/blob/fe0278e36ea9768b7c51e1899f9c1b34e39c5025/lucene/core/src/java/org/apache/lucene/util/packed/DeltaPackedLongValues.java#L85-L91
Maybe we should merge these two loops too? (Separately: somehow consolidate these two paths)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jpountz commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1599116196
Have we checked if this actually made things faster? I remember getting surprised in the past because folding too much into a single loop would prevent C2 from recognizing thas some bits can be auto-vectorized (like computing the min value across the entire array).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] easyice commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "easyice (via GitHub)" <gi...@apache.org>.
easyice commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1600339243
@mikemccand @jpountz Thank you for your suggestions and fresh perspectives on this change, i wrote a simply benchmark for DirectMonotonicWriter, it will write 500 blocks each loop and observe the minimum time taken, the results appear to be slightly faster, from 492ms->425ms
here is the benchmark:
```
public class IndexBenchMarks {
public static void main(final String[] args) throws Exception {
doWriteMonotonic();
}
static void doWriteMonotonic() throws IOException {
BenchMark benchMark = new BenchMark(50, 50, (1 << BenchMark.DIRECT_MONOTONIC_BLOCK_SHIFT) * 500);
benchMark.run();
}
static class BenchMark {
final int warmup;
final int numValues;
final int loopCount;
static final int DIRECT_MONOTONIC_BLOCK_SHIFT = 16;
Directory dir;
DirectMonotonicWriter writer;
IndexOutput metaOut;
IndexOutput dataOut;
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);
metaOut = dir.createOutput("meta", IOContext.DEFAULT);
dataOut = dir.createOutput("data", IOContext.DEFAULT);
}
private void close() throws IOException {
metaOut.close();
dataOut.close();
dir.close();
}
private void doWrite() throws IOException {
long v = 100;
for (int i = 0; i < numValues; i++) {
if (i % 2 == 0) {
v += 5;
} else {
v += 10;
}
writer.add(v);
}
}
void run() throws IOException {
init();
for (int i = 0; i < warmup; i++) {
writer = DirectMonotonicWriter.getInstance(metaOut, dataOut, numValues, DIRECT_MONOTONIC_BLOCK_SHIFT);
doWrite();
writer.finish();
}
System.gc();
List<Double> times = new ArrayList<>();
for (int i = 0; i < loopCount; i++) {
writer = DirectMonotonicWriter.getInstance(metaOut, dataOut, numValues, DIRECT_MONOTONIC_BLOCK_SHIFT);
long t0 = System.nanoTime();
doWrite();
writer.finish();
times.add((System.nanoTime() - t0) / 1000000D);
}
double min = times.stream().mapToDouble(Number::doubleValue).min().getAsDouble();
System.out.println("took(ms):" + String.format(Locale.ROOT, "%.2f", min));
close();
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] easyice commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "easyice (via GitHub)" <gi...@apache.org>.
easyice commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1598374932
@gf2121 Thanks for you quick reply, the CHANGES.txt has updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1598727681
I wonder if this will move the needle in [Lucene's nightly benchmarks](https://home.apache.org/~mikemccand/lucenebench/)? Let's watch the charts after this is merged ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jpountz commented on pull request #12377: Avoid redundant loop for compute min value in DirectMonotonicWriter
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #12377:
URL: https://github.com/apache/lucene/pull/12377#issuecomment-1599392439
I'm fine either way, hopefully this bit of code is not a bottleneck anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org