uschindler commented on PR #15104: URL: https://github.com/apache/lucene/pull/15104#issuecomment-3213188047
> > So this is not testing mmapdir's implementation at all! > > https://github.com/apache/lucene/blob/6cfaa5ef831cb361909e32a621ac9ecc6514c7cc/lucene/benchmark-jmh/src/java/org/apache/lucene/benchmark/jmh/GroupVIntBenchmark.java#L187-L191 > > > > So actually we have no bechhmark and this shows why the performance trap with the lambda was never visible in any benchmark!? > > In my understanding, this PR should fix this issue as well. > > In the main branch, `benchMMapDirectoryInputs_readGroupVInt` invokes readGroupVInt(DataInput, **long[]**, int), which bypasses `in.readGroupVInt` and therefore does not exercise the mmap code path. > > This PR updates `benchMMapDirectoryInputs_readGroupVInt` to call readGroupVInts(DataInput, **int[]**, int) instead, ensuring that the mmap code is covered. Hi, it was a bit late yesterday. The problem I mainly have is with the variable naming. It talks about ByteBuffers at many places, but actually it is about MMap. - Can we fix this in this PR, so it is clear what each benchmark is doing? I am good at reading and understanding code if at least variables and field names are senseful, but the current state of that benchmark is unreadable and therefor I want to have a rewrite with less methods and variable named that make sense (e.g, "mmapIndexInput" instead of "byteBufferGVIntIn") - We should also remove the "dead" long[] code in GroupVIntUtil (the one which was targeted for DataInputSubclasses for optimization). - We should check the setup of the benchmark, to me it looks like warmup times and runtimes are much too small to they are very noisy. @jpountz found out that since by change in https://github.com/apache/lucene/pull/15089 the direct benchmaek was slower, but in Mike's production benchmark it showed a significant speedup for queries. - We should have benchmakrs for three impls: NIOFSDir (uses my new code with long->int downcast), ByteBuffersDir (uses my new code, same as NIOFSDir), MMAPDir (uses my new code) Once we have the bench fixed, let's compare the results pre/post https://github.com/apache/lucene/pull/15089 again! Thanks, Uwe -- 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-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
