You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/09/27 11:01:25 UTC
[GitHub] [pinot] richardstartin opened a new pull request #7487: use MethodHandle to acces vectorized unsigned comparison on JDK9+
richardstartin opened a new pull request #7487:
URL: https://github.com/apache/pinot/pull/7487
## Description
This allows vectorized `byte[]` comparison on JDK9+. This was motivated by #7405 but `ByteArray.compare` is used in a lot of places, so this should be a big win.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin edited a comment on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin edited a comment on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-928305819
Here are some numbers from `BenchmarkStringDictionary` which hits `ByteArray.compare` twice on the write path but not on the read path. I threw away the number for the off-heap dictionary because they are noisy, and used a fixed seed so subsequent runs are comparable - this change looks worthwhile.
```
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 39.954 ± 2.446 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 49.021 ± 10.705 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 48.543 ± 12.200 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 53.494 ± 7.755 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 68.075 ± 15.418 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 66.177 ± 5.318 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.576 ± 1.738 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 70.257 ± 7.173 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 117.377 ± 5.491 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 115.491 ± 8.360 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 116.254 ± 14.534 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 120.159 ± 8.665 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 142.495 ± 12.102 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 196.842 ± 12.584 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 210.740 ± 16.198 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 229.852 ± 16.526 ms/op
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 38.390 ± 2.110 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 39.827 ± 7.020 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 46.284 ± 12.710 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 58.891 ± 11.733 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 56.999 ± 3.481 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 68.284 ± 3.805 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.878 ± 1.086 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 75.076 ± 1.144 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 100.520 ± 5.884 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 108.455 ± 4.986 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 107.341 ± 5.383 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 113.251 ± 4.794 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 122.947 ± 5.376 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 154.681 ± 4.031 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 152.651 ± 6.078 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 151.176 ± 5.072 ms/op
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r717053717
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +115,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
We did that in the roaringbitmap library, which is a lot simpler than Pinot, but setting it up so we could actually test both implementations was very complicated. This way we get to test both implementations just by building with different JDKs (there is currently a profile to build with JDK8)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gunnarmorling commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
gunnarmorling commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r717016022
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +115,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
Might be a nice use case for keeping two flavours of this method in a multi-release JAR? You'd avoid the method handle business altogether, at the price of a slightly more complex build set-up.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin edited a comment on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin edited a comment on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-928305819
Here are some numbers from `BenchmarkStringDictionary` which hits `ByteArray.compare` twice on the write path but not on the read path. I threw away the number for the off-heap dictionary because they are noisy, and used a fixed seed so subsequent runs are comparable - this change looks worthwhile.
```
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 39.954 ± 2.446 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 49.021 ± 10.705 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 48.543 ± 12.200 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 53.494 ± 7.755 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 68.075 ± 15.418 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 66.177 ± 5.318 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.576 ± 1.738 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 70.257 ± 7.173 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 117.377 ± 5.491 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 115.491 ± 8.360 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 116.254 ± 14.534 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 120.159 ± 8.665 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 142.495 ± 12.102 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 196.842 ± 12.584 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 210.740 ± 16.198 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 229.852 ± 16.526 ms/op
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 38.390 ± 2.110 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 39.827 ± 7.020 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 46.284 ± 12.710 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 58.891 ± 11.733 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 56.999 ± 3.481 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 68.284 ± 3.805 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.878 ± 1.086 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 75.076 ± 1.144 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 100.520 ± 5.884 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 108.455 ± 4.986 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 107.341 ± 5.383 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 113.251 ± 4.794 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 122.947 ± 5.376 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 154.681 ± 4.031 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 152.651 ± 6.078 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 151.176 ± 5.072 ms/op
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716736742
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -31,6 +34,21 @@
* </ul>
*/
public class ByteArray implements Comparable<ByteArray>, Serializable {
+
+ private static final MethodHandle COMPARE_UNSIGNED;
+
+ static {
+ MethodHandle compareUnsigned = null;
+ try {
+ compareUnsigned = MethodHandles.publicLookup().findStatic(Arrays.class, "compareUnsigned",
+ MethodType.methodType(int.class,
+ byte[].class, int.class, int.class,
+ byte[].class, int.class, int.class));
+ } catch (NoSuchMethodException | IllegalAccessException ignore) {
Review comment:
Only on JDK8 AFAIK, but let's log it 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin edited a comment on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin edited a comment on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-927795364
Here's a quick comparison to show what's at stake here (I don't think the benchmark is worth maintaining in this project). Note that I selected lengths which aren't aligned to make sure slower tail loops execute, which won't happen on aligned inputs.
```java
@State(Scope.Benchmark)
public class BenchmarkByteArrayCompare {
@Param({"7", "31", "1023"})
int _length;
@Param("1")
int _offset;
private byte[] _left;
private byte[] _right;
@Setup(Level.Trial)
public void setup() {
_left = new byte[_length];
ThreadLocalRandom.current().nextBytes(_left);
_right = Arrays.copyOf(_left, _left.length);
_right[_right.length - _offset] = (byte) ~_left[_left.length - _offset];
}
@Benchmark
public int compare() {
return ByteArray.compare(_left, _right);
}
}
```
```
Benchmark (_length) (_offset) Mode Cnt Score Error Units
BenchmarkByteArrayCompare.compare 7 1 avgt 5 5.942 ± 0.048 ns/op
BenchmarkByteArrayCompare.compare 31 1 avgt 5 15.298 ± 1.040 ns/op
BenchmarkByteArrayCompare.compare 1023 1 avgt 5 320.390 ± 1.262 ns/op
Benchmark (_length) (_offset) Mode Cnt Score Error Units
BenchmarkByteArrayCompare.compare 7 1 avgt 5 7.913 ± 0.174 ns/op
BenchmarkByteArrayCompare.compare 31 1 avgt 5 8.351 ± 0.016 ns/op
BenchmarkByteArrayCompare.compare 1023 1 avgt 5 22.765 ± 1.566 ns/op
```
For inputs shorter than 8 bytes, there is a small setup cost of the order of 1ns, but for larger inputs the improvement is huge.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
walterddr commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716777570
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +109,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
i dunno how java byte code were compiled but if you are catching throwable and ignore below. could we just ignore null check here? :-p
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-928305819
Here are some numbers from `BenchmarkStringDictionary` which hits `ByteArray.compare` on both the write and read path. I threw away the number for the off-heap dictionary because they are noisy, and used a fixed seed so subsequent runs are comparable - this change looks worthwhile.
```
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 39.954 ± 2.446 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 49.021 ± 10.705 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 48.543 ± 12.200 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 53.494 ± 7.755 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 68.075 ± 15.418 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 66.177 ± 5.318 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.576 ± 1.738 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 70.257 ± 7.173 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 117.377 ± 5.491 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 115.491 ± 8.360 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 116.254 ± 14.534 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 120.159 ± 8.665 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 142.495 ± 12.102 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 196.842 ± 12.584 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 210.740 ± 16.198 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 229.852 ± 16.526 ms/op
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 38.390 ± 2.110 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 39.827 ± 7.020 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 46.284 ± 12.710 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 58.891 ± 11.733 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 56.999 ± 3.481 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 68.284 ± 3.805 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.878 ± 1.086 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 75.076 ± 1.144 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 100.520 ± 5.884 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 108.455 ± 4.986 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 107.341 ± 5.383 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 113.251 ± 4.794 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 122.947 ± 5.376 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 154.681 ± 4.031 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 152.651 ± 6.078 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 151.176 ± 5.072 ms/op
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #7487:
URL: https://github.com/apache/pinot/pull/7487
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716820519
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +109,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
+ try {
+ return (int) COMPARE_UNSIGNED.invokeExact(left, leftFromIndex, leftToIndex,
+ right, rightFromIndex, rightToIndex);
+ } catch (ArrayIndexOutOfBoundsException outOfBounds) {
+ throw outOfBounds;
+ } catch (Throwable ignore) {
Review comment:
tests here https://github.com/apache/pinot/pull/7487/commits/efa0e25504391f19ce6e76c4357cfdf6be666664
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-927789663
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7487](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ecdeb09) into [master](https://codecov.io/gh/apache/pinot/commit/400056804fc1c0da73c5ec78ca3fe1a1bcab9231?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4000568) will **decrease** coverage by `6.89%`.
> The diff coverage is `75.47%`.
> :exclamation: Current head ecdeb09 differs from pull request most recent head dfbf935. Consider uploading reports for the commit dfbf935 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7487 +/- ##
============================================
- Coverage 72.02% 65.13% -6.90%
- Complexity 3369 3408 +39
============================================
Files 1519 1477 -42
Lines 75364 73654 -1710
Branches 10987 10804 -183
============================================
- Hits 54279 47971 -6308
- Misses 17434 22272 +4838
+ Partials 3651 3411 -240
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `69.82% <75.48%> (-0.05%)` | :arrow_down: |
| unittests2 | `14.50% <1.88%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...not/segment/local/customobject/QuantileDigest.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9jdXN0b21vYmplY3QvUXVhbnRpbGVEaWdlc3QuamF2YQ==) | `58.70% <0.00%> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `82.96% <0.00%> (-5.93%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `85.29% <0.00%> (-1.71%)` | :arrow_down: |
| [.../io/params/InvertedSortedIndexJointRuleParams.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pby9wYXJhbXMvSW52ZXJ0ZWRTb3J0ZWRJbmRleEpvaW50UnVsZVBhcmFtcy5qYXZh) | `34.88% <50.00%> (ø)` | |
| [.../index/loader/invertedindex/RangeIndexHandler.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9pbnZlcnRlZGluZGV4L1JhbmdlSW5kZXhIYW5kbGVyLmphdmE=) | `42.00% <50.00%> (+0.33%)` | :arrow_up: |
| [...ain/java/org/apache/pinot/spi/utils/ByteArray.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZUFycmF5LmphdmE=) | `55.26% <50.00%> (-29.36%)` | :arrow_down: |
| [...gment/index/readers/BitSlicedRangeIndexReader.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvQml0U2xpY2VkUmFuZ2VJbmRleFJlYWRlci5qYXZh) | `74.07% <74.07%> (ø)` | |
| [...local/segment/creator/impl/inv/MmapFileWriter.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvTW1hcEZpbGVXcml0ZXIuamF2YQ==) | `90.90% <90.90%> (ø)` | |
| [...t/creator/impl/inv/BitSlicedRangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvQml0U2xpY2VkUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `91.83% <91.83%> (ø)` | |
| [...org/apache/pinot/client/DynamicBrokerSelector.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0R5bmFtaWNCcm9rZXJTZWxlY3Rvci5qYXZh) | `90.00% <100.00%> (-4.88%)` | :arrow_down: |
| ... and [376 more](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4000568...dfbf935](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gunnarmorling commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
gunnarmorling commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r717238421
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +115,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
Yeah, build/testing set-up is a bit more complex for sure. Anyways, just thought I'd bring it up, looks like you found a satisfying solution in any case.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
walterddr commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716776370
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +109,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
+ try {
+ return (int) COMPARE_UNSIGNED.invokeExact(left, leftFromIndex, leftToIndex,
+ right, rightFromIndex, rightToIndex);
+ } catch (ArrayIndexOutOfBoundsException outOfBounds) {
+ throw outOfBounds;
+ } catch (Throwable ignore) {
Review comment:
can we add tests for the IOE and fallback for JDK9+?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r719375301
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +115,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
To do this with a Multi-Release jar you need two versions of the class `foo.bar.MultiVersionClass`, one compiled at `--release 8`, using only JDK8 APIs, and the other using `Arrays.compareUnsigned` at `--release 9`, and they need to be packaged in different directories. The JDK8 class file should be at `/foo/bar/MultiVersionClass.class` and the JDK9 one at `META-INF/versions/9/foo/bar/MultiVersionClass.class`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] gunnarmorling commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
gunnarmorling commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r717016022
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +115,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
Might be a nice use case for keeping two flavours of this method in a multi-release JAR? You'd avoid the method handle business altogether, at the price of a slightly more complex build set-up.
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +115,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
Yeah, build/testing set-up is a bit more complex for sure. Anyways, just thought I'd bring it up, looks like you found a satisfying solution in any case.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r717053717
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +115,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
We did that in the roaringbitmap library, which is a lot simpler than Pinot, but setting it up so we could actually test both implementations was very complicated. This way we get to test both implementations just by building with different JDKs (there is currently a profile to build with JDK8)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
walterddr commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716777570
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +109,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
i dunno how java byte code were compiled but if you are catching throwable and ignore below. could we just ignore null check here? would that be faster? :-p
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716781276
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +109,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
The difference is the throwable caught below will never actually happen, it's just an aspect of the `MethodHandle` API needing to expose potentially any checked exception. We certainly don't want a `NullPointerException` thrown on every invocation on JDK8.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-927789663
# [Codecov](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#7487](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ecdeb09) into [master](https://codecov.io/gh/apache/pinot/commit/400056804fc1c0da73c5ec78ca3fe1a1bcab9231?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4000568) will **decrease** coverage by `2.19%`.
> The diff coverage is `75.48%`.
> :exclamation: Current head ecdeb09 differs from pull request most recent head dfbf935. Consider uploading reports for the commit dfbf935 to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7487 +/- ##
============================================
- Coverage 72.02% 69.82% -2.20%
+ Complexity 3369 3328 -41
============================================
Files 1519 1129 -390
Lines 75364 53399 -21965
Branches 10987 8043 -2944
============================================
- Hits 54279 37285 -16994
+ Misses 17434 13467 -3967
+ Partials 3651 2647 -1004
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `69.82% <75.48%> (-0.05%)` | :arrow_down: |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...not/segment/local/customobject/QuantileDigest.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9jdXN0b21vYmplY3QvUXVhbnRpbGVEaWdlc3QuamF2YQ==) | `58.70% <0.00%> (ø)` | |
| [...ent/index/column/PhysicalColumnIndexContainer.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2NvbHVtbi9QaHlzaWNhbENvbHVtbkluZGV4Q29udGFpbmVyLmphdmE=) | `82.96% <0.00%> (-5.93%)` | :arrow_down: |
| [...al/segment/index/readers/RangeIndexReaderImpl.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvUmFuZ2VJbmRleFJlYWRlckltcGwuamF2YQ==) | `85.29% <0.00%> (-1.71%)` | :arrow_down: |
| [.../index/loader/invertedindex/RangeIndexHandler.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9pbnZlcnRlZGluZGV4L1JhbmdlSW5kZXhIYW5kbGVyLmphdmE=) | `42.00% <50.00%> (+0.33%)` | :arrow_up: |
| [...ain/java/org/apache/pinot/spi/utils/ByteArray.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZUFycmF5LmphdmE=) | `55.26% <50.00%> (-29.36%)` | :arrow_down: |
| [...gment/index/readers/BitSlicedRangeIndexReader.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvQml0U2xpY2VkUmFuZ2VJbmRleFJlYWRlci5qYXZh) | `74.07% <74.07%> (ø)` | |
| [...local/segment/creator/impl/inv/MmapFileWriter.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvTW1hcEZpbGVXcml0ZXIuamF2YQ==) | `90.90% <90.90%> (ø)` | |
| [...t/creator/impl/inv/BitSlicedRangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvQml0U2xpY2VkUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `91.83% <91.83%> (ø)` | |
| [...al/segment/creator/impl/inv/RangeIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9pbnYvUmFuZ2VJbmRleENyZWF0b3IuamF2YQ==) | `80.63% <100.00%> (ø)` | |
| [...local/segment/index/loader/IndexLoadingConfig.java](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9JbmRleExvYWRpbmdDb25maWcuamF2YQ==) | `64.83% <100.00%> (+0.58%)` | :arrow_up: |
| ... and [614 more](https://codecov.io/gh/apache/pinot/pull/7487/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [4000568...dfbf935](https://codecov.io/gh/apache/pinot/pull/7487?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-928305819
Here are some numbers from `BenchmarkStringDictionary` which hits `ByteArray.compare` on both the write and read path. I threw away the number for the off-heap dictionary because they are noisy, and used a fixed seed so subsequent runs are comparable - this change looks worthwhile.
```
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 39.954 ± 2.446 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 49.021 ± 10.705 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 48.543 ± 12.200 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 53.494 ± 7.755 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 68.075 ± 15.418 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 66.177 ± 5.318 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.576 ± 1.738 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 70.257 ± 7.173 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 117.377 ± 5.491 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 115.491 ± 8.360 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 116.254 ± 14.534 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 120.159 ± 8.665 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 142.495 ± 12.102 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 196.842 ± 12.584 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 210.740 ± 16.198 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 229.852 ± 16.526 ms/op
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 38.390 ± 2.110 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 39.827 ± 7.020 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 46.284 ± 12.710 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 58.891 ± 11.733 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 56.999 ± 3.481 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 68.284 ± 3.805 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.878 ± 1.086 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 75.076 ± 1.144 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 100.520 ± 5.884 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 108.455 ± 4.986 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 107.341 ± 5.383 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 113.251 ± 4.794 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 122.947 ± 5.376 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 154.681 ± 4.031 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 152.651 ± 6.078 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 151.176 ± 5.072 ms/op
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin removed a comment on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin removed a comment on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-928305819
Here are some numbers from `BenchmarkStringDictionary` which hits `ByteArray.compare` twice on the write path but not on the read path. I threw away the number for the off-heap dictionary because they are noisy, and used a fixed seed so subsequent runs are comparable - this change looks worthwhile.
```
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 39.954 ± 2.446 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 49.021 ± 10.705 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 48.543 ± 12.200 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 53.494 ± 7.755 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 68.075 ± 15.418 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 66.177 ± 5.318 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.576 ± 1.738 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 70.257 ± 7.173 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 117.377 ± 5.491 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 115.491 ± 8.360 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 116.254 ± 14.534 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 120.159 ± 8.665 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 142.495 ± 12.102 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 196.842 ± 12.584 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 210.740 ± 16.198 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 229.852 ± 16.526 ms/op
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 38.390 ± 2.110 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 39.827 ± 7.020 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 46.284 ± 12.710 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 58.891 ± 11.733 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 56.999 ± 3.481 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 68.284 ± 3.805 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.878 ± 1.086 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 75.076 ± 1.144 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 100.520 ± 5.884 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 108.455 ± 4.986 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 107.341 ± 5.383 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 113.251 ± 4.794 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 122.947 ± 5.376 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 154.681 ± 4.031 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 152.651 ± 6.078 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 151.176 ± 5.072 ms/op
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716790993
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +109,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
+ try {
+ return (int) COMPARE_UNSIGNED.invokeExact(left, leftFromIndex, leftToIndex,
+ right, rightFromIndex, rightToIndex);
+ } catch (ArrayIndexOutOfBoundsException outOfBounds) {
+ throw outOfBounds;
+ } catch (Throwable ignore) {
Review comment:
Good call - turns out we don't have a test for this class at all 😬
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] tivrfoa commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
tivrfoa commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r719368071
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +115,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
Review comment:
Hi folks!
I was going to ask the question below, but then I understood why MethodHandle lookup was used.
Very nice idea! Multi-release jar is very nice too! =)
---
Newbie question:<br>
I need to compile with java version >= 9 to use Arrays.compareUnsigned, but if I run with Java 8 I get:<br>
```
Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.UnsupportedClassVersionError: JavaVersion has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0
or
bad class file: ./JavaVersion.class
class file has wrong version 55.0, should be 52.0
Please remove or make sure it appears in the correct subdirectory of the classpath.
```
So what's the point to prepare the code for a previous version?<br>
ps: I'm sure I'm missing something. =D
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-927795364
Here's a quick comparison to show what's at stake here (I don't think the benchmark is worth maintaining in this project). Note that I selected lengths which aren't aligned to make sure slower tails of loops execute, which won't happen on aligned inputs.
```java
@State(Scope.Benchmark)
public class BenchmarkByteArrayCompare {
@Param({"7", "31", "1023"})
int _length;
@Param("1")
int _offset;
private byte[] _left;
private byte[] _right;
@Setup(Level.Trial)
public void setup() {
_left = new byte[_length];
ThreadLocalRandom.current().nextBytes(_left);
_right = Arrays.copyOf(_left, _left.length);
_right[_right.length - _offset] = (byte) ~_left[_left.length - _offset];
}
@Benchmark
public int compare() {
return ByteArray.compare(_left, _right);
}
}
```
```
Benchmark (_length) (_offset) Mode Cnt Score Error Units
BenchmarkByteArrayCompare.compare 7 1 avgt 5 5.942 ± 0.048 ns/op
BenchmarkByteArrayCompare.compare 31 1 avgt 5 15.298 ± 1.040 ns/op
BenchmarkByteArrayCompare.compare 1023 1 avgt 5 320.390 ± 1.262 ns/op
Benchmark (_length) (_offset) Mode Cnt Score Error Units
BenchmarkByteArrayCompare.compare 7 1 avgt 5 7.913 ± 0.174 ns/op
BenchmarkByteArrayCompare.compare 31 1 avgt 5 8.351 ± 0.016 ns/op
BenchmarkByteArrayCompare.compare 1023 1 avgt 5 22.765 ± 1.566 ns/op
```
For inputs shorter than 8 bytes, there is a small setup cost of the order of 1ns, but for larger inputs the improvement is huge.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716736347
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +109,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
+ try {
+ return (int) COMPARE_UNSIGNED.invokeExact(left, leftFromIndex, leftToIndex,
+ right, rightFromIndex, rightToIndex);
+ } catch (ArrayIndexOutOfBoundsException outOfBounds) {
+ throw outOfBounds;
+ } catch (Throwable ignore) {
Review comment:
it can't happen, it's just a consequence of the API. Propagating `ArrayIndexOutOfBoundsException` is important though.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] richardstartin removed a comment on pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
richardstartin removed a comment on pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#issuecomment-928305819
Here are some numbers from `BenchmarkStringDictionary` which hits `ByteArray.compare` twice on the write path but not on the read path. I threw away the number for the off-heap dictionary because they are noisy, and used a fixed seed so subsequent runs are comparable - this change looks worthwhile.
```
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 39.954 ± 2.446 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 49.021 ± 10.705 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 48.543 ± 12.200 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 53.494 ± 7.755 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 68.075 ± 15.418 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 66.177 ± 5.318 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.576 ± 1.738 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 70.257 ± 7.173 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 117.377 ± 5.491 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 115.491 ± 8.360 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 116.254 ± 14.534 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 120.159 ± 8.665 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 142.495 ± 12.102 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 196.842 ± 12.584 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 210.740 ± 16.198 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 229.852 ± 16.526 ms/op
Benchmark (_maxValueLength) (_seed) Mode Cnt Score Error Units
BenchmarkStringDictionary.onHeapStringDictionaryRead 8 2021 avgt 15 38.390 ± 2.110 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 16 2021 avgt 15 39.827 ± 7.020 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 32 2021 avgt 15 46.284 ± 12.710 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 64 2021 avgt 15 58.891 ± 11.733 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 128 2021 avgt 15 56.999 ± 3.481 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 256 2021 avgt 15 68.284 ± 3.805 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 512 2021 avgt 15 68.878 ± 1.086 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryRead 1024 2021 avgt 15 75.076 ± 1.144 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 8 2021 avgt 15 100.520 ± 5.884 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 16 2021 avgt 15 108.455 ± 4.986 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 32 2021 avgt 15 107.341 ± 5.383 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 64 2021 avgt 15 113.251 ± 4.794 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 128 2021 avgt 15 122.947 ± 5.376 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 256 2021 avgt 15 154.681 ± 4.031 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 512 2021 avgt 15 152.651 ± 6.078 ms/op
BenchmarkStringDictionary.onHeapStringDictionaryWrite 1024 2021 avgt 15 151.176 ± 5.072 ms/op
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] kishoreg commented on a change in pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #7487:
URL: https://github.com/apache/pinot/pull/7487#discussion_r716730806
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -91,19 +109,55 @@ public int compareTo(@Nonnull ByteArray that) {
* <li> +ve integer if first value is larger than the second. </li>
* </ul>
*
- * @param bytes1 First byte[] to compare.
- * @param bytes2 Second byte[] to compare.
+ * @param left First byte[] to compare.
+ * @param right Second byte[] to compare.
+ * @return Result of comparison as stated above.
+ */
+ public static int compare(byte[] left, byte[] right) {
+ return compare(left, 0, left.length, right, 0, right.length);
+ }
+
+ /**
+ * Compares two byte[] values. The comparison performed is on unsigned value for each byte.
+ * Returns:
+ * <ul>
+ * <li> 0 if both values are identical. </li>
+ * <li> -ve integer if first value is smaller than the second. </li>
+ * <li> +ve integer if first value is larger than the second. </li>
+ * </ul>
+ *
+ * @param left First byte[] to compare.
+ * @param leftFromIndex inclusive index of first byte to compare in left
+ * @param leftToIndex exclusive index of last byte to compare in left
+ * @param right Second byte[] to compare.
+ * @param rightFromIndex inclusive index of first byte to compare in right
+ * @param rightToIndex exclusive index of last byte to compare in right
* @return Result of comparison as stated above.
*/
- public static int compare(byte[] bytes1, byte[] bytes2) {
- int len1 = bytes1.length;
- int len2 = bytes2.length;
+ public static int compare(byte[] left, int leftFromIndex, int leftToIndex,
+ byte[] right, int rightFromIndex, int rightToIndex) {
+ if (COMPARE_UNSIGNED != null) {
+ try {
+ return (int) COMPARE_UNSIGNED.invokeExact(left, leftFromIndex, leftToIndex,
+ right, rightFromIndex, rightToIndex);
+ } catch (ArrayIndexOutOfBoundsException outOfBounds) {
+ throw outOfBounds;
+ } catch (Throwable ignore) {
Review comment:
when would this happen, should we propagate it
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/utils/ByteArray.java
##########
@@ -31,6 +34,21 @@
* </ul>
*/
public class ByteArray implements Comparable<ByteArray>, Serializable {
+
+ private static final MethodHandle COMPARE_UNSIGNED;
+
+ static {
+ MethodHandle compareUnsigned = null;
+ try {
+ compareUnsigned = MethodHandles.publicLookup().findStatic(Arrays.class, "compareUnsigned",
+ MethodType.methodType(int.class,
+ byte[].class, int.class, int.class,
+ byte[].class, int.class, int.class));
+ } catch (NoSuchMethodException | IllegalAccessException ignore) {
Review comment:
will be good to log a warning that we cannot apply vectorization, I am guessing this would happen with jdk8 or some security restriction?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #7487: use MethodHandle to access vectorized unsigned comparison on JDK9+
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #7487:
URL: https://github.com/apache/pinot/pull/7487
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org