uschindler commented on code in PR #15116:
URL: https://github.com/apache/lucene/pull/15116#discussion_r2295466666
##########
lucene/core/src/java/org/apache/lucene/util/GroupVIntUtil.java:
##########
@@ -57,13 +59,44 @@ public static void readGroupVInts(DataInput in, int[] dst,
int limit) throws IOE
* @param offset the offset in the array to start storing ints.
*/
public static void readGroupVInt(DataInput in, int[] dst, int offset) throws
IOException {
+ readGroupVInt(true, in, dst, offset);
+ }
+
+ /** DO not use! Only visible for benchmarking purposes! */
+ public static void readGroupVInt$Baseline(DataInput in, int[] dst, int
offset)
+ throws IOException {
+ readGroupVInt(false, in, dst, offset);
+ }
+
+ private static void readGroupVInt(boolean optimized, DataInput in, int[]
dst, int offset)
+ throws IOException {
final int flag = in.readByte() & 0xFF;
final int n1Minus1 = flag >> 6;
final int n2Minus1 = (flag >> 4) & 0x03;
final int n3Minus1 = (flag >> 2) & 0x03;
final int n4Minus1 = flag & 0x03;
+ // if our DataInput implements RandomAccessInput for absolute access and
IndexInput for seeking,
+ // we use a branch-less implementation:
+ if (optimized && in instanceof RandomAccessInput rin && in instanceof
IndexInput iin) {
Review Comment:
Basically if you are afraid of the two instanceof checks which do not seem
to have an impact if all is optimized (which is the case for the benchmark as
it is isolated with only exactly one implementation), then my original idea was:
Instead of having GroupVIntUtil a purely static class, i'd make in have 2
implementation (base class sealed + abstract). This is not only cleaner to
implement and we don't need to expose the baseline, but we can also make the
code compile and inline even better, because for reading you could then
allocate an instance for reading GroupVInts on top of DataInput when you open
the index once. It is just a wrapper around the IndexInput. There will be
exactly 2 implementations available, so the overhead is minimal.
Basically in all users of this class (codecs), we do:
```java
// open index file
final (Data|Index)Input input = ....
final GroupVIntUtil gvintInput = GroupVIntUtil.getReadInstance(input);
// code that reads:
gvintInput.readGroupVInt(....)
```
The static `getReadInstance()` method would return one of both
implementations that is optimized or not. To benchmark baseline we can add a
method to retun an unoptimized instance on request.
That would fit the current usage patterns perfectly.
--
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
