Skip to content

optimize the read lock caused by computeIfAbsent and synchronized in high concurrency #9980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
philippzhang wants to merge 1 commit into from
Closed

optimize the read lock caused by computeIfAbsent and synchronized in high concurrency #9980

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions core/trino-spi/src/main/java/io/trino/spi/type/TypeOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,15 @@ public class TypeOperators
public TypeOperators()
{
ConcurrentHashMap<Object, Object> cache = new ConcurrentHashMap<>();
this.cache = (operatorConvention, supplier) -> cache.computeIfAbsent(operatorConvention, key -> supplier.get());
// optimize the read lock caused by computeIfAbsent
// this.cache = (operatorConvention, supplier) -> cache.computeIfAbsent(operatorConvention, key -> supplier.get());
this.cache = (operatorConvention, supplier) -> {
Object value = cache.get(operatorConvention);
if (value == null) {
value = cache.computeIfAbsent(operatorConvention, key -> supplier.get());
Copy link
Copy Markdown
Member

@lhofhansl lhofhansl Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this makes a difference.

Edit: In a microbenchmark I find this indeed to be about 1/3 of the cost when the key is present. In this case we never remove any entries, so it is safe.

Copy link
Copy Markdown
Author

@philippzhang philippzhang Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During large-scale data testing, we found that this code has performance problems. When modifying, we also refer to the transformation idea of computeifabsent method of mybatis. The following is the modification of mybatis:

mybatis/mybatis-3#2223

Copy link
Copy Markdown
Member

@hashhar hashhar Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linked JDK bug was fixed in JDK 9 (see https://bugs.openjdk.java.net/browse/JDK-8161372) Trino requires JDK 11 at-least. Is there some way to reproduce your test to observe the locking?

Copy link
Copy Markdown
Member

@lhofhansl lhofhansl Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My microbenchmark was with JDK 11.

Edit: There is still some extra locking needed to enforce the exactly-once execution of the compute part when the key is concurrently removed.
Hence when you know keys are never removed get() followed by computeIfAbsent might be faster.

Copy link
Copy Markdown
Member

@sopel39 sopel39 Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to doc:

Some attempted update
     * operations on this map by other threads may be blocked while
     * computation is in progress, so the computation should be short
     * and simple.

looking at java.util.concurrent.ConcurrentHashMap#computeIfAbsent it shouldn't lock if element is already present, so once cache is filled, there should be no locking.

Why do you think this change matters in non-benchmark executions

Copy link
Copy Markdown
Author

@philippzhang philippzhang Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During large-scale data testing, we found that this code has performance problems. When modifying, we also refer to the transformation idea of computeifabsent method of mybatis. The following is the modification of mybatis:

mybatis/mybatis-3#2223

Copy link
Copy Markdown
Member

@sopel39 sopel39 Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During large-scale data testing, we found that this code has performance problems.

How did you do the testing?
io.trino.spi.type.TypeOperators#cache should not be changed after a while since all operators would be cached. Hence, there should be no locking

}
return value;
};
}

public TypeOperators(BiFunction<Object, Supplier<Object>, Object> cache)
Expand Down Expand Up @@ -181,10 +189,14 @@ public OperatorAdaptor(ScalarFunctionAdapter functionAdapter, OperatorConvention
this.operatorConvention = operatorConvention;
}

public synchronized MethodHandle get()
// optimize the read lock caused by synchronized
// public synchronized MethodHandle get()
public MethodHandle get()
{
if (adapted == null) {
Copy link
Copy Markdown
Member

@lhofhansl lhofhansl Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adapted would have to be declared volatile, no?

Copy link
Copy Markdown
Author

@philippzhang philippzhang Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, this is to avoid locking when reading data. During the TB level data test, it is found that this code has a blocked lock. If modified, it can provide certain performance when reading data

Copy link
Copy Markdown
Member

@lhofhansl lhofhansl Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. But it still needs to be volatile to be correct. Since that has a slight performance implication (a memory barrier for each access), it might be prudent to redo the perf test.

Copy link
Copy Markdown
Member

@martint martint Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. without volatile, this code is not safe according to Java Memory Model semantics. The thread may perceive operations in the wrong order and see a partially constructed object. This is the classic "double checked locking" pattern.

adapted = adaptOperator(operatorConvention);
synchronized (this) {
adapted = adaptOperator(operatorConvention);
}
}
return adapted;
}
Expand Down