Skip to content
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.

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

Mapper method invocation should be non-blocking (work around JDK-8161372) #1929

Closed
lixinwen opened this issue May 17, 2020 · 4 comments · Fixed by #1934
Closed

Mapper method invocation should be non-blocking (work around JDK-8161372) #1929

lixinwen opened this issue May 17, 2020 · 4 comments · Fixed by #1934
Assignees
Labels
Milestone

Comments

@lixinwen
Copy link

lixinwen commented May 17, 2020

MyBatis version

3.5.4

Database vendor and version

Test case or example project

MapperProxy#cachedInvoker {methodCache.computeIfAbsent}
In ConcurrentHashMap#computeIfAbsent, the following code will cause lock acquire:
[
else if ((fh = f.hash) == MOVED)
tab = helpTransfer(tab, f);
else {
boolean added = false;
// Here, even if the method and corresponding MapperMethodInvoker already exist
synchronized (f) {
]
I think, for mybatis, this lock is not necessary, maybe should change the code like follow:
[
MapperMethodInvoker invoker = methodCache.get(method);
if (invoker == null) {
MapperMethodInvoker newInvoker = .......
MapperMethodInvoker old = methodCache.putIfAbsent(newInvoker);
if (old != null) {
invoker = old;
}
}
return invoker;
]

Steps to reproduce

Expected result

Actual result

@harawata
Copy link
Member

harawata commented May 17, 2020

That is a JDK's issue rather than MyBatis'.
And it actually is fixed in JDK 9 and the backport to JDK 8 is also planned, it seems.

As MyBatis still supports Java 8, I thought about adding a preliminary check as you proposed, but it didn't make much difference (here is the JMH project that I used).
Do you have any benchmark data proving otherwise?

[EDIT]
Well, the benchmark should be done using multiple threads, obviously.
I will re-test later.

@harawata
Copy link
Member

harawata commented May 18, 2020

@lixinwen ,

I updated the JMH project and verified the effect of the preliminary check under heavy load.
Does #1934 look OK to you?

@lixinwen
Copy link
Author

lixinwen commented May 19, 2020

@harawata
Thank you, this what i want.

@harawata harawata changed the title [BUG] MapperProxy#cachedInvoker, Acquire the lock in the ConcurrentHashMap#computeIfAbsent every time Mapper method invocation should be non-blocking (work around JDK-8161372) May 19, 2020
@harawata harawata self-assigned this May 19, 2020
@harawata harawata added this to the 3.5.5 milestone May 19, 2020
@harawata
Copy link
Member

harawata commented May 19, 2020

You can verify the fix with 3.5.5-SNAPSHOT.
Thank you for taking the time to report, @lixinwen !

harawata added a commit to harawata/mybatis-3 that referenced this issue Apr 9, 2021
…fAbsent()

This is a workaround for JDK-8161372

mybatis#1929
https://groups.google.com/g/mybatis-user/c/iBTN98c0dP4/m/H6IJS-LzAQAJ

We should drop this workaround once we drop Java 8 support or the fix is backported.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants