You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "original-brownbear (via GitHub)" <gi...@apache.org> on 2024/09/21 10:43:04 UTC
[PR] Fix contention in StringHelper.randomId [lucene]
original-brownbear opened a new pull request, #13816:
URL: https://github.com/apache/lucene/pull/13816
Seeing this mutex contended up to O(10ms) in Elasticsearch at times. Moving to CAS and removing the unnecessary alloction of a new instance for the bitwise-and with the mask makes this perform+scale much better.
--
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
Re: [PR] Fix contention in StringHelper.randomId [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #13816:
URL: https://github.com/apache/lucene/pull/13816#issuecomment-2365228413
this ID is generated for safety, and we only calculate this **once per segment** when initially flushing it. Doesn't even happen at merge. Really, it should be a few insns under lock, shouldn't need to be fast. That's why there isn't contention, your profiler is lying to you. Sorry, I don't buy 30ms.
And if somehow you are writing THAT many segments concurrently, then you definitely need the safety (and we dont want thread visibility issues where 2 segments end out with the same ID).
If we want to improve it, IMO the best way is to remove custom BigDecimal and xor/shift logic, but instead swap in the OpenJDK implementation. This code was written before OpenJDK had decent PRNGs.
This will clean up the code AND improve the performance.
```java
// initialization
var factory = RandomGeneratorFactory.of("Xoroshiro128PlusPlus");
var generator = factory.create(seed);
// generation
byte[] result = new byte[ID_LENGTH];
synchronized(idLock) {
random.nextBytes(result);
}
return result;
```
Instead of two BigDecimal operations and an allocation under the lock, it will just do some xors and shifts on two `long` variables behind the scenes. And I think I would trust it more since code is maintained in the JDK.
--
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
Re: [PR] Fix contention in StringHelper.randomId [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on PR #13816:
URL: https://github.com/apache/lucene/pull/13816#issuecomment-2365187862
> profilers often lie about this
True but here I'd say whatever number the profiler outputs is only part of the cost of hard synchronizing on a global like we do here (we also have the indirect cost from cache/memory effects). Plus what the profiler isn't wrong about is the fact that threads actually go into waiting in this spot when benchmarking systems with large core/thread counts.
That said: If I can't convince you of the CAS fix. How about adjusting the math like I did here and moving the `toByteArray` call out of the `synchronized` block? That has pretty much the same effect because the code in the sychronized becomes fast enough to remove the thread-waiting in my testing.
--
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
Re: [PR] Fix contention in StringHelper.randomId [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #13816:
URL: https://github.com/apache/lucene/pull/13816#issuecomment-2365147464
profilers often lie about this. I think we should keep the synchronization simple. the IDs are important to not duplicate.
--
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
Re: [PR] Fix contention in StringHelper.randomId [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13816:
URL: https://github.com/apache/lucene/pull/13816#issuecomment-3408723821
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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