You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "sandynz (via GitHub)" <gi...@apache.org> on 2023/03/12 03:57:09 UTC
[GitHub] [shardingsphere] sandynz opened a new pull request, #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
sandynz opened a new pull request, #24558:
URL: https://github.com/apache/shardingsphere/pull/24558
There might be `IllegalStateException: Recursive update` if `ShardingSphereServiceLoader.getServiceInstances` is called recursively. e.g.
```
Caused by: java.lang.IllegalStateException: Recursive update
at java.base/java.util.concurrent.ConcurrentHashMap.transfer(ConcurrentHashMap.java:2552)
at java.base/java.util.concurrent.ConcurrentHashMap.addCount(ConcurrentHashMap.java:2354)
at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1776)
at org.apache.shardingsphere.infra.util.spi.ShardingSphereServiceLoader.getServiceInstances(ShardingSphereServiceLoader.java:71)
```
After discussion with @TeslaCN , it's better to remove `computeIfAbsent`.
Changes proposed in this pull request:
- Replace computeIfAbsent in getServiceInstances to avoid possible ConcurrentHashMap recursive update
- Lazy load service instances to reduce locked time in getServiceInstances
---
Before committing this PR, I'm sure that I have checked the following options:
- [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [ ] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [ ] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [ ] I have made corresponding changes to the documentation.
- [ ] I have added corresponding unit tests for my changes.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN commented on PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#issuecomment-1478857397
`Recursive update` in ConcurrentHashMap is similar with `java.util.ConcurrentModificationException`. Replacing `compute` methods cannot resolve the root cause.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "sandynz (via GitHub)" <gi...@apache.org>.
sandynz commented on PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#issuecomment-1481036942
If users load SPI recursively when customizing SPI, I don't think it's users' fault (root cause).
e.g.
```
public final class OuterSPI implements TypedSPI {
// TypedSPILoader depends on ShardingSphereServiceLoader
private final InnerSPI innerSPI = TypedSPILoader.getService(InnerSPI.class, "type");
}
```
Users just use classes of ShardingSphere indirectly, e.g. `ShardingSphereServiceLoader`, but not use `ConcurrentHashMap` directly.
And `ShardingSphereServiceLoader.getServiceInstances` javadoc doesn't declare it could not be invoked recursively.
Maybe we could update javadoc of `ShardingSphereServiceLoader` and also [Dev Manual]( https://shardingsphere.apache.org/document/5.3.1/en/dev-manual/ ).
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on a diff in pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN commented on code in PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#discussion_r1209664884
##########
infra/util/src/main/java/org/apache/shardingsphere/infra/util/spi/ShardingSphereServiceLoader.java:
##########
@@ -70,7 +80,17 @@ private Collection<T> load() {
@SuppressWarnings("unchecked")
public static <T> Collection<T> getServiceInstances(final Class<T> serviceInterface) {
ShardingSphereServiceLoader<?> result = LOADERS.get(serviceInterface);
- return (Collection<T>) (null != result ? result.getServiceInstances() : LOADERS.computeIfAbsent(serviceInterface, ShardingSphereServiceLoader::new).getServiceInstances());
+ if (null != result) {
+ return (Collection<T>) result.getServiceInstances();
+ }
+ synchronized (LOAD_LOCKS[serviceInterface.hashCode() % LOAD_LOCKS_COUNT]) {
Review Comment:
Got it.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] codecov-commenter commented on pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#issuecomment-1465089824
## [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/24558?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#24558](https://codecov.io/gh/apache/shardingsphere/pull/24558?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e7c502d) into [master](https://codecov.io/gh/apache/shardingsphere/commit/7c022a41dedf76d19de906fe8ae340682fcaf40a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7c022a4) will **increase** coverage by `0.01%`.
> The diff coverage is `92.59%`.
> :exclamation: Current head e7c502d differs from pull request most recent head 1352bd4. Consider uploading reports for the commit 1352bd4 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24558 +/- ##
============================================
+ Coverage 50.00% 50.01% +0.01%
+ Complexity 1595 1594 -1
============================================
Files 3311 3311
Lines 54047 54065 +18
Branches 9942 9945 +3
============================================
+ Hits 27024 27042 +18
+ Misses 24606 24605 -1
- Partials 2417 2418 +1
```
| [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/24558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...re/infra/util/spi/ShardingSphereServiceLoader.java](https://codecov.io/gh/apache/shardingsphere/pull/24558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-aW5mcmEvdXRpbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvaW5mcmEvdXRpbC9zcGkvU2hhcmRpbmdTcGhlcmVTZXJ2aWNlTG9hZGVyLmphdmE=) | `92.85% <92.59%> (-2.98%)` | :arrow_down: |
... and [1 file with indirect coverage changes](https://codecov.io/gh/apache/shardingsphere/pull/24558/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "sandynz (via GitHub)" <gi...@apache.org>.
sandynz commented on code in PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#discussion_r1209657748
##########
infra/util/src/main/java/org/apache/shardingsphere/infra/util/spi/ShardingSphereServiceLoader.java:
##########
@@ -70,7 +80,17 @@ private Collection<T> load() {
@SuppressWarnings("unchecked")
public static <T> Collection<T> getServiceInstances(final Class<T> serviceInterface) {
ShardingSphereServiceLoader<?> result = LOADERS.get(serviceInterface);
- return (Collection<T>) (null != result ? result.getServiceInstances() : LOADERS.computeIfAbsent(serviceInterface, ShardingSphereServiceLoader::new).getServiceInstances());
+ if (null != result) {
+ return (Collection<T>) result.getServiceInstances();
+ }
+ synchronized (LOAD_LOCKS[serviceInterface.hashCode() % LOAD_LOCKS_COUNT]) {
Review Comment:
Sorry, I missed this message before.
`serviceInterface` might be synchronized here and in other places, I'm afraid there might be synchronization issue, so I use dedicated lock here.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "sandynz (via GitHub)" <gi...@apache.org>.
sandynz commented on PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#issuecomment-1469825958
I tested different implementations with JMH
- Replace ConcurrentHashMap to ConcurrentSkipListMap, one line changed (not committed). Performance is bad than before.
- Use ConcurrentHashMap, lazy load (Commit reverted). Performance is a little bad than before.
- Use ConcurrentHashMap, small range locks. Performance is similar as before.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "sandynz (via GitHub)" <gi...@apache.org>.
sandynz commented on PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#issuecomment-1469837704
JMH 1.36 test result.
Result:
```
Benchmark Mode Cnt Score Error Units
ShardingSphereServiceLoaderJMHTest.loadNewInstanceV1 thrpt 5 23.109 ± 2.709 ops/us
ShardingSphereServiceLoaderJMHTest.loadNewInstanceV2 thrpt 5 24.156 ± 0.694 ops/us
ShardingSphereServiceLoaderJMHTest.loadSingletonInstanceV1 thrpt 5 134.726 ± 4.417 ops/us
ShardingSphereServiceLoaderJMHTest.loadSingletonInstanceV2 thrpt 5 132.867 ± 17.676 ops/us
```
Steps:
1. Copy original code of ShardingSphereServiceLoader.java into ShardingSphereServiceLoader1.java
2. Run ShardingSphereServiceLoaderJMHTest.java to test singleton instances and new instances loading
Add dependencies in shardingsphere-infra-util module pom.xml:
```
<dependency>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-core</artifactId>
<version>1.36</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.openjdk.jmh</groupId>
<artifactId>jmh-generator-annprocess</artifactId>
<version>1.36</version>
<scope>test</scope>
</dependency>
```
Add ShardingSphereServiceLoaderJMHTest.java in `shardingsphere-infra-util` module:
```
package org.apache.shardingsphere.infra.util.spi;
import org.apache.shardingsphere.infra.util.spi.fixture.singleton.SingletonSPIFixture;
import org.apache.shardingsphere.infra.util.spi.type.typed.fixture.TypedSPIFixture;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.RunnerException;
import org.openjdk.jmh.runner.options.Options;
import org.openjdk.jmh.runner.options.OptionsBuilder;
import java.util.concurrent.TimeUnit;
@BenchmarkMode(Mode.Throughput)
@OutputTimeUnit(TimeUnit.MICROSECONDS)
@State(Scope.Benchmark)
public class ShardingSphereServiceLoaderJMHTest {
public static void main(String[] args) throws RunnerException {
Options options = new OptionsBuilder().include(ShardingSphereServiceLoaderJMHTest.class.getSimpleName())
.forks(1).warmupIterations(5).measurementIterations(5)
.build();
new Runner(options).run();
}
@Benchmark
public void loadSingletonInstanceV1() {
ShardingSphereServiceLoader1.getServiceInstances(SingletonSPIFixture.class);
}
@Benchmark
public void loadSingletonInstanceV2() {
ShardingSphereServiceLoader.getServiceInstances(SingletonSPIFixture.class);
}
@Benchmark
public void loadNewInstanceV1() {
ShardingSphereServiceLoader1.getServiceInstances(TypedSPIFixture.class);
}
@Benchmark
public void loadNewInstanceV2() {
ShardingSphereServiceLoader.getServiceInstances(TypedSPIFixture.class);
}
}
```
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN commented on PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#issuecomment-1502685472
> If users load SPI recursively when customizing SPI, I don't think it's users' fault (root cause).
>
> e.g.
>
> ```
> public final class OuterSPI implements TypedSPI {
>
> // TypedSPILoader depends on ShardingSphereServiceLoader
> private final InnerSPI innerSPI = TypedSPILoader.getService(InnerSPI.class, "type");
> }
> ```
>
> Users just use classes of ShardingSphere indirectly, e.g. `ShardingSphereServiceLoader`, but not use `ConcurrentHashMap` directly. And `ShardingSphereServiceLoader.getServiceInstances` javadoc doesn't declare it could not be invoked recursively.
>
> Maybe we could update javadoc of `ShardingSphereServiceLoader` and also [Dev Manual](https://shardingsphere.apache.org/document/5.3.1/en/dev-manual/).
Got it.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN commented on a diff in pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN commented on code in PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558#discussion_r1162298379
##########
infra/util/src/main/java/org/apache/shardingsphere/infra/util/spi/ShardingSphereServiceLoader.java:
##########
@@ -70,7 +80,17 @@ private Collection<T> load() {
@SuppressWarnings("unchecked")
public static <T> Collection<T> getServiceInstances(final Class<T> serviceInterface) {
ShardingSphereServiceLoader<?> result = LOADERS.get(serviceInterface);
- return (Collection<T>) (null != result ? result.getServiceInstances() : LOADERS.computeIfAbsent(serviceInterface, ShardingSphereServiceLoader::new).getServiceInstances());
+ if (null != result) {
+ return (Collection<T>) result.getServiceInstances();
+ }
+ synchronized (LOAD_LOCKS[serviceInterface.hashCode() % LOAD_LOCKS_COUNT]) {
Review Comment:
How about `synchronized (serviceInterface)`?
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] TeslaCN merged pull request #24558: Avoid ConcurrentHashMap recursive update in ShardingSphereServiceLoader.getServiceInstances
Posted by "TeslaCN (via GitHub)" <gi...@apache.org>.
TeslaCN merged PR #24558:
URL: https://github.com/apache/shardingsphere/pull/24558
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org