gortiz commented on code in PR #12794:
URL: https://github.com/apache/pinot/pull/12794#discussion_r1559066330
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/unsafe/DirectMemory.java:
##########
@@ -55,23 +54,18 @@ public void flush() {
}
@Override
- public void close()
- throws IOException {
+ public synchronized void close() {
if (!_closed) {
- synchronized (this) {
- if (!_closed) {
- Unsafer.UNSAFE.freeMemory(_address);
- _closed = true;
- }
- }
+ Unsafer.UNSAFE.freeMemory(_address);
Review Comment:
You are right, I didn't see that. Anyway, in the other PR I've changed that
so we don't need to synchronize in case one thread close this after another.
What newer jvms does for a fact is to make synchronized by default is
specially expensive. This is due to:
* Virtual threads being pinned with synchronize. This is secondary given we
don't use them and it should be something that eventually will be improved in
(even) more modern JVMs
* +UseBiasedLocking has been disabled since Java 15 and the plan is to
remove it. See https://openjdk.org/jeps/374,
[this](https://dev.to/vbochenin/java-17-migration-bias-locks-regression-2c5m#:~:text=What%20is%20a%20bias%20lock,the%20same%20thread%20is%20lightweight.)
and [this](https://quarkus.io/blog/biased-locking-help/)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
