You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "JarvisCraft (via GitHub)" <gi...@apache.org> on 2023/05/14 23:57:59 UTC
[GitHub] [lucene] JarvisCraft opened a new pull request, #12290: Make memory fence in `ByteBufferGuard` explicit
JarvisCraft opened a new pull request, #12290:
URL: https://github.com/apache/lucene/pull/12290
### Description
Current implementation of `ByteBufferGuard` uses an implementation-based assumption that `AtomicInteger#lazySet` causes full-barrrier (ie `[prior stores] happen before [following loads & stores]` is required).
Since `VarHandle` appearance there have been explicit methods for performing memory fences including full fence thus it seems reasonable to rely on them rather than ungaranteed semantic.
It's worth mentioning that unlike #9824 which proposes to use new explicit access modes on the field, this does not change the type of `invalidated` or its access methods but simply replaces the old non-obviously invoked barrier with an obvious one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1549806649
> but actually this class should go away at some point!
Yup, but as for now it seems more safe to rely on well-defined behaviour (until this class is gone).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1549844375
The whole `ByteBufferGuard` does not help too much to prevent code from failing and crushing the VM! But yes, it's a good idea to enforce more.
I played with that in the past: `ByteBufferGuard` never helped much, a `full fence` neither. This class was bullshit from the beginning as it only delayed the crushing of JVM a bit more on misuse.
So it should never have existed...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1549878245
Had to go back and re-read what all this was about in the beginning. I like the patch (nice API) but sort of agree with Uwe - the current solution is flawed (but working 99% of the time). The patch wouldn't change it to work correctly but it has the potential of having unknown side effects... Let's just keep the existing solution until a truly sound one is available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1549864252
> Sorry,
> I'd suggest to not touch ByteBufferGuard at all,
No problems! Was glad to hear your review on this change)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1549785157
I like it, but actually this class should go away at some point!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1549857185
Sorry,
I'd suggest to not touch `ByteBufferGuard` at all, it did its job to throw AlreadyClosedException instead of segmentation fault consistently until memory access is compiled by Hotspot. Don't touch it.
W have to live with it only until Project Panama is in mainline JDKs. Unfortunatley as of today, not even 21 will ship with non-preview Panama, so tricky compilation to prevent preview warnings in #12294 is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] sherman commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "sherman (via GitHub)" <gi...@apache.org>.
sherman commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1549883140
I think it's a good idea to replace this tricky way to flush any CPU caches, such as using lazy set, with an explicit full fence. The purpose of a full fence is to provide a reliable memory barrier that works consistently across different architectures.
But if you're quite certain that the days of this class are numbered, then it's fine.
(By the way, we are still planning to test it in our implementation of the directory interface).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dweiss commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1547186814
Didn't know about it, but I like it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12290:
URL: https://github.com/apache/lucene/pull/12290#discussion_r1195248137
##########
lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java:
##########
@@ -39,7 +40,7 @@ final class ByteBufferGuard {
* this to allow unmapping of bytebuffers with private Java APIs.
*/
@FunctionalInterface
- static interface BufferCleaner {
+ interface BufferCleaner {
Review Comment:
be explicit here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1571747521
> Please also add a CHANGES.txt entry in 9.7 section.
All done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1571897316
Applied and cherry-picked on 9.x branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on a diff in pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on code in PR #12290:
URL: https://github.com/apache/lucene/pull/12290#discussion_r1208663284
##########
lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java:
##########
@@ -65,14 +62,8 @@ public ByteBufferGuard(String resourceDescription, BufferCleaner cleaner) {
public void invalidateAndUnmap(ByteBuffer... bufs) throws IOException {
if (cleaner != null) {
invalidated = true;
- // This call should hopefully flush any CPU caches and as a result make
- // the "invalidated" field update visible to other threads. We specifically
- // don't make "invalidated" field volatile for performance reasons, hoping the
- // JVM won't optimize away reads of that field and hardware should ensure
- // caches are in sync after this call. This isn't entirely "fool-proof"
- // (see LUCENE-7409 discussion), but it has been shown to work in practice
- // and we count on this behavior.
- barrier.lazySet(0);
+ // Makes "invalidated" field visible to other threads.
+ VarHandle.fullFence();
Review Comment:
My apologies for the late response! I will update the PR according to your review in this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on a diff in pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on code in PR #12290:
URL: https://github.com/apache/lucene/pull/12290#discussion_r1212913125
##########
lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java:
##########
@@ -65,14 +62,8 @@ public ByteBufferGuard(String resourceDescription, BufferCleaner cleaner) {
public void invalidateAndUnmap(ByteBuffer... bufs) throws IOException {
if (cleaner != null) {
invalidated = true;
- // This call should hopefully flush any CPU caches and as a result make
- // the "invalidated" field update visible to other threads. We specifically
- // don't make "invalidated" field volatile for performance reasons, hoping the
- // JVM won't optimize away reads of that field and hardware should ensure
- // caches are in sync after this call. This isn't entirely "fool-proof"
- // (see LUCENE-7409 discussion), but it has been shown to work in practice
- // and we count on this behavior.
- barrier.lazySet(0);
+ // Makes "invalidated" field visible to other threads.
+ VarHandle.fullFence();
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler merged PR #12290:
URL: https://github.com/apache/lucene/pull/12290
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12290:
URL: https://github.com/apache/lucene/pull/12290#discussion_r1199073297
##########
lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java:
##########
@@ -65,14 +62,8 @@ public ByteBufferGuard(String resourceDescription, BufferCleaner cleaner) {
public void invalidateAndUnmap(ByteBuffer... bufs) throws IOException {
if (cleaner != null) {
invalidated = true;
- // This call should hopefully flush any CPU caches and as a result make
- // the "invalidated" field update visible to other threads. We specifically
- // don't make "invalidated" field volatile for performance reasons, hoping the
- // JVM won't optimize away reads of that field and hardware should ensure
- // caches are in sync after this call. This isn't entirely "fool-proof"
- // (see LUCENE-7409 discussion), but it has been shown to work in practice
- // and we count on this behavior.
- barrier.lazySet(0);
+ // Makes "invalidated" field visible to other threads.
+ VarHandle.fullFence();
Review Comment:
I'd like to merge this, but could we restore most of the previous comment.
The comment added is not correct, because the invalidated field is not definitely visible to other threads (only in code not yet optimized).
So please refer to the "fool-proof" discussion too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] JarvisCraft commented on a diff in pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "JarvisCraft (via GitHub)" <gi...@apache.org>.
JarvisCraft commented on code in PR #12290:
URL: https://github.com/apache/lucene/pull/12290#discussion_r1195269692
##########
lucene/core/src/java/org/apache/lucene/store/ByteBufferGuard.java:
##########
@@ -39,7 +40,7 @@ final class ByteBufferGuard {
* this to allow unmapping of bytebuffers with private Java APIs.
*/
@FunctionalInterface
- static interface BufferCleaner {
+ interface BufferCleaner {
Review Comment:
Rolled back, although I feel like `static` does not add to expliicitness here considering that interfaces never can be non-static; also, most code seems to omit `static` on inner `interface`s: 23 without it VS 4 with it (including this one).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12290: Make memory fence in `ByteBufferGuard` explicit
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12290:
URL: https://github.com/apache/lucene/pull/12290#issuecomment-1549935792
I tried the full fence a year ago, it did not help when code is optimized by hotspot. For sure it throws AlreadyClosedException when running in interpreted mode (so it helps users to figure out that their code is broken during testing). When in production, it is too late.
I will keep this open a few days. I would merge it (and also backport it), because I agree with @sherman: it can't get worse. The static method in VarHandle does not have any side effects, it just makes explicit what the AtomicInteger code did before implicit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org