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

[philippzhang]

We test on hundreds of trino based on TB data, support sql query through YJP (YourKit-JavaProfiler), and find that there is a blocked lock of threads. This is the image query performance. After removing the lock, the query performance has a certain improvement. Let’s look at the code. It is found that these locks can not be locked when reading data, and the lock is retained when writing data.

before change code:

after change code:

eg:
sql SELECT SUM(a), SUM(b), COUNT(c), SUM(pv), SUM(id) FROM table where event_day = '$day' and eg_code = $code

[cla-bot]

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: zhangyangshuo.
This is most likely caused by a git client misconfiguration; please make sure to:

1. check if your git client is configured with an email to sign commits git config --list | grep email
2. If not, set it up using git config --global user.email email@example.com
3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

[lhofhansl]

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.

[philippzhang]

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

[hashhar]

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?

[lhofhansl]

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.

[lhofhansl]

adapted would have to be declared volatile, no?

[philippzhang]

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

[lhofhansl]

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.

[martint]

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.

[sopel39]

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

[philippzhang]

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

[sopel39]

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

[bitsondatadev]

👋 @philippzhang - this PR is inactive and doesn't seem to be under development. If you'd like to continue work on this at any point in the future, feel free to re-open.