[GitHub] [lucene] dweiss commented on issue #11925: error-prone's JVM arguments need help
dweiss commented on issue #11925: URL: https://github.com/apache/lucene/issues/11925#issuecomment-1312666964 I think you want to pass a jvm configuring flag to javac, assuming JavaCompile is in fork mode (which is always the case in Lucene). errorprone hacks into javac, so I think it'll work just fine. What this means is that you have to configure JavaCompile (like we do in other places) and pass "internal" options to it - this seems to work for me, for example: ``` javac -J-XX:ActiveProcessorCount=1 -J-XX:+PrintFlagsFinal --help ``` I'm not at my dev machine but I can try later. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
uschindler commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1020891869
##
lucene/core/src/java/org/apache/lucene/util/BytesRefHash.java:
##
@@ -414,7 +414,7 @@ private void rehash(final int newSize, boolean hashOnData) {
}
hashMask = newMask;
-bytesUsed.addAndGet(Integer.BYTES * (-ids.length));
Review Comment:
This is always a bit hard to read. Mabye we should add a method
`Counter#subtractAndGet` in our counter class? I think this might be a separate
issue, but might worth to do it.
An alternative to this line: In most cases in this issue you applied the
cast to "constants" (because then the javac compiler can do the inlining
directly (you know, javac inlines static final integer and String literals). So
maybe have it here like `(long) Integer.BYTES * (-ids.length)`.
Sorry for bringing this specific place up again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] rmuir commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
rmuir commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1020892411
##
lucene/core/src/java/org/apache/lucene/util/BytesRefHash.java:
##
@@ -414,7 +414,7 @@ private void rehash(final int newSize, boolean hashOnData) {
}
hashMask = newMask;
-bytesUsed.addAndGet(Integer.BYTES * (-ids.length));
Review Comment:
i reverted the last commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #11923: enable error-prone "narrow calculation" check
rmuir commented on PR #11923: URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312717162 just hash out how you want the style of the bytesrefhash code to be as we are going back and forth here. a little sad as i thought spotless was the end of the java style bikesheds :) ill check back in a few days. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #11923: enable error-prone "narrow calculation" check
uschindler commented on PR #11923: URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312717445 @rmuir just some question about cases like `merge.estimatedMergeBytes / 1024 / 1024.` changed to `merge.estimatedMergeBytes / 1024. / 1024.`: Were they also found by this check or did you just change them because you have ssen them. Because the pattern here is different. Division with integers should not overflow, so I wonder why this was found by the errorprone check. The new code is much better as it is consistent! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #11923: enable error-prone "narrow calculation" check
uschindler commented on PR #11923: URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312717771 All fine! I vote: merge this ASAP :-) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #11923: enable error-prone "narrow calculation" check
uschindler commented on PR #11923: URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312718213 > @rmuir just some question about cases like `merge.estimatedMergeBytes / 1024 / 1024.` changed to `merge.estimatedMergeBytes / 1024. / 1024.`: Were they also found by this check or did you just change them because you have ssen them. Because the pattern here is different. Division with integers should not overflow, so I wonder why this was found by the errorprone check. The new code is much better as it is consistent! A sorry, yes it is a bug for small integers. It is explained in the errorprone bug docs: https://errorprone.info/bugpattern/NarrowCalculation Sorry for asking! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] uschindler opened a new pull request, #11926: Improve test introduced in #11918 to also check that reported invalid position is transformed back original position by slicing code
uschindler opened a new pull request, #11926: URL: https://github.com/apache/lucene/pull/11926 This improves the test a bit, especially regarding this code (and I'm glad: code is fine): - https://github.com/apache/lucene/blob/98b26e0885dcc4dc0b7a085f5d23d5099d59808a/lucene/core/src/java/org/apache/lucene/store/ByteBufferIndexInput.java#L626-L630 - https://github.com/apache/lucene/blob/98b26e0885dcc4dc0b7a085f5d23d5099d59808a/lucene/core/src/java19/org/apache/lucene/store/MemorySegmentIndexInput.java#L548-L552 It also uses smaller file with fixed chunk size to reduce I/O. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #11926: Improve test introduced in #11918 to also check that reported invalid position is transformed back original position by slicing code
uschindler merged PR #11926: URL: https://github.com/apache/lucene/pull/11926 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] dweiss commented on issue #11925: error-prone's JVM arguments need help
dweiss commented on issue #11925: URL: https://github.com/apache/lucene/issues/11925#issuecomment-1312774179 I think this is what you wanted, @rmuir https://github.com/apache/lucene/pull/11927 This tweaks jvm options of every javac task. I think the slowdown you're observing is not just because of jvm options but a side-effect of the fact that running with spotless triggers all of those javac tasks to be forked (they mess with bootclasspath, I think). By default javac runs in-process with gradle and this is faster on my machine (regardless of what jvm options I pass). If you take a peek at the patch, you can force everything to be forked but I'm not sure if it makes sense. Hope it helps! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #11927: pass jvm args to javac #11925
rmuir commented on PR #11927: URL: https://github.com/apache/lucene/pull/11927#issuecomment-1312774986 I will try it out. I tried something similar to this... maybe exactly this... but when i inspected the forked processes for error-prone with `ps` they weren't actually getting the flags. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] dweiss commented on pull request #11927: pass jvm args to javac #11925
dweiss commented on PR #11927: URL: https://github.com/apache/lucene/pull/11927#issuecomment-1312798750 I can see jvm flags being dumped if I add ```-XX:+PrintFlagsFinal``` to this patch (and either force the fork mode or run with an alt. jvm, which has the same side-effect). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] mdmarshmallow commented on issue #11868: Add a FilterIndexOutput
mdmarshmallow commented on issue #11868: URL: https://github.com/apache/lucene/issues/11868#issuecomment-1312954877 Sounds good, I just started working on this and will make sure to add `FilterIndexInput` as well. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
