By Jeroen Borgers (jPinpoint) and Peter Paul Bakker (Stokpop), sponsored by Rabobank
- Introduction
- Improper back-end interaction / remote service calls
- Improper asynchrony
- Improper caching
- Too much session usage
- Reloading lists of values
- Inefficient database access
- Improper use of XML and remoting
- Improper use of JSON and remoting
- Using XPath
- Improper logging
- Improper Streaming I/O
- Extensive use of classpath scanning
- Unnecessary use of reflection
- Thread unsafety and lock contention
- Unnecessary execution
- Inefficient memory usage
- Improper use of collections
- Inefficient String usage
- Inefficient date time formatting
- Inefficient regular expression usage
- Use of slow library calls
- Potential memory leaks
- Violation of Encapsulation, DRY or SRP
We categorized many performance pitfalls based on our findings in practice in the last decade. Sources we used are: performance code reviews, load and stress tests, heap analyses,
profiling and production problems of various applications of companies. We present these pitfalls with best practice solutions as follows.
Several of these pitfalls are automated into custom PMD/Sonar jPinpoint-rules.
Disclaimer: these best practices come without warranty of any kind. Use at your own risk and always (load)test in your own situation.
The most important factor of application performance is the number of back-end service calls. Therefore, minimizing this number and avoiding identical calls must have top priority. Downside of the use of ‘independent’ front-end elements is the difficulty to avoid identical service requests from multiple frontend elements on the same page or on different pages. This can be both for the same application/ear as well as for separate applications/ears.
Observation: Multiple service requests and cache requests are executed for the same actual back-end service request and multiple, possibly slightly different, cache entries occupy the cache(s).
Problem: Response times are higher than needed, load on the back-end is higher than needed and there is more data in the cache(s) than needed, to result in less efficiency: more load on the cache/database or more overload to disk of the in-memory cache.
Solution 1: If the service is called on most front-end elements, this could be solved on application level.
Solution 2: The integration layer could be shared on source level between applications/ears, such that the same service calls and cache and cache entries are used.
Observation: Back-end calls are executed in a loop without a hardcoded stop criterion, e.g. by a 'there is more' flag returned from the back-end.
Problem: If the back-end has a failure and returns a 'there is more' flag incorrectly, or the two parties think differently about this flag, this results in back-end responses being accumulated in the Java heap during the loop. After a while this will result in an out-of-memory situation.
Solution: Introduce a sensible hard-coded limit to the number of iterations. This is better than a limit by a property, since properties may be configured incorrectly, and go unnoticed if they are not used in the happy flow.
Observation: The HTTP connection manager uses the default maximum connections per route.
Problem: The default maximum connections per route is by default set to 2. This throttles the number of connections to the back-end usually much more than required,
such that many requests have to wait for a connection and response times get much higher than needed.
The connection timeout is usually set to a low number (e.g. 300 ms), in that case connection timeout exceptions will occur.
Solution: Set the default maximum connections per route to a higher number, e.g. 20.
Also increase the Max Total to at least the DefaultMaxPerRoute or a multiple in case of multiple routes.
Example for only one host. Can be done in two ways:
- via the
PoolingHttpClientConnectionManager:
PoolingHttpClientConnectionManager connMgr = new PoolingHttpClientConnectionManager();
connMgr.setMaxTotal(maxConnections);
connMgr.setDefaultMaxPerRoute(maxConnections);
CloseableHttpClient httpClient = HttpClients.custom()
.setConnectionManager(connMgr)
.build();- or directly on the client via e.g.
HttpClientsbuilder:
CloseableHttpClient httpClient = HttpClients.custom()
.setMaxConnPerRoute(MAX_CONNECTIONS_PER_ROUTE)
.setMaxConnTotal(MAX_CONNECTIONS_TOTAL)
.build();or for asynchronous connections using nio:
private HttpAsyncClientBuilder createHttpClientConfigCallback(final HttpAsyncClientBuilder clientBuilder) {
clientBuilder
.setMaxConnPerRoute(MAX_CONNECTIONS_PER_ROUTE)
.setMaxConnTotal(MAX_CONNECTIONS_TOTAL);Note that class PoolingClientConnectionManager and several others are deprecated and PoolingHttpClientConnectionManager is the one to use for synchronous calls and NHttpClientConnectionManager (e.g. by HttpAsyncClientBuilder) for asynchronous calls.
Rule name: HttpClientBuilderWithoutPoolSize.
Pool size calculation
The pool size must generally not be blocking, it should have enough connections for long yet still valid service times, during peak load.
For calculating the pool size, we suggest the following formula:
#connections = 1,5 * #requests/s (at peak load) * service time (at 95th percentile, in seconds)
So, with a peak load of 10 requests/s and the 95th percentile of the time of the called service of 1,2 second, this gets:
#connections = 1,5 * 10 * 1,2 = 18
If you don't know the peak load or the 95th percentile service time, estimate these values.
With high load like 200 tps, in practice, service times need to be very short in order for the service to work properly. The #connections should typically be limited to < 60.
See also: Thread pool sizing
Moved to Improper asychrony category
Observation: Apache DefaultHttpClient is used with multiple threads.
Problem: This HttpClient can only handle one thread on one connection and will throw an IllegalStateException in case a second thread tries to connect when the first is using the connection.
Solution: Use a PoolingHttpConnectionManager, see Apache doc: connection management.
Observation: Deprecated or thread-unsafe HTTP connectors are used.
Problem: Several HTTP client connection managers are thread-unsafe which may cause session data mix-up or have other issues for which they were made deprecated. Highest risk of session data mixup: SimpleHttpConnectionManager.
Other deprecated ones to remove: SimpleHttpConnectionManager, ClientConnectionManager, PoolingClientConnectionManager, ThreadSafeClientConnManager, SingleClientConnManager, DefaultHttpClient, SystemDefaultHttpClient, ClientConnectionManager, MultiThreadedHttpConnectionManager, HttpClient constructor.
Solution: Use org.apache.http.impl.conn.PoolingHttpClientConnectionManager and org.apache.http.impl.client.HttpClientBuilder., see Apache doc: connection management.
Rule name: AvoidDeprecatedHttpConnectors.
Observation: The HTTP client has enabled connection state tracking while using TLS.
Problem: HTTP connections between services are mostly stateless, meaning there is no specific user identity or security context for each session. Exceptions are NTLM authenticated connections and SSL/TLS connections with client certificate authentication. If HttpClients have enabled connection state tracking which is the default, established TLS connections will not be reused because it is assumed that the user identity or security context may differ. Then performance will suffer due to a full TLS handshake for each request.
Solution: HttpClients should disable connection state tracking in order to reuse TLS connections, since service calls for one pool have the same user identity/security context for all sessions. Connect to one back-end only from one pool. See Apache Http client tutorial
Example (correct):
@Bean(name = "saveObjectReferencesHttpClient")
public HttpClient httpClient(@Qualifier("saveObjectReferencesConnectionManager") PoolingHttpClientConnectionManager connectionManager,
SaveObjectReferenceConnectionProperties connectionProperties) {
return HttpClientBuilder.create()
.setConnectionManager(connectionManager)
.setDefaultRequestConfig(RequestConfigUtils.builder(connectionProperties))
.disableConnectionState() // allow re-use of mutual authenticated TLS connections
.build();
}Observation: HttpClient is used instead of ClosableHttpClient.
Problem: if HttpClient connections are not closed properly when needed, resources are not released and connections may not (or not quick enough) become available from the pool.
Solution: Use ClosableHttpClient to allow for invoking close on it to properly close the connection. Or use HttpComponentsClientHttpRequestFactory(httpClient) and let it manage closing.
Rule name: UseCloseableForHttpClient
Example:
void bad() {
HttpClient httpClient = HttpClientBuilder.create()
.disableConnectionState().build();
}
void good() {
CloseableHttpClient httpClient = HttpClientBuilder.create()
.disableConnectionState().build();
}
void good2() {
HttpClient httpClient = HttpClientBuilder.create().disableConnectionState().build();
ClientHttpRequestFactory clientHttpRequestFactory = new HttpComponentsClientHttpRequestFactory(httpClient);
} For more information, see httpclient-connection-management
Observation: HttpClient builder is used with a ConnectionManager and setMaxConnTotal and/or setMaxConnPerRoute is called on the client.
Problem: If you use setConnectionManager, the connection pool must be configured on that Connection Manager. Pool settings on the client are ignored and lost. This is confusing and a source of errors.
Solution: HttpClients should either:
- use setConnectionManager and only configure the pool on the connection manager or
- not use a ConnectionManager and configure the connection pool on the client.
So, if you use a Connection Manager, remove the setMaxConnTotal and setMaxConnPerRoute calls on the HttpClient.
Rule name: HttpClientBuilderPoolSettingsIgnored
Example:
return HttpClientBuilder.create()
.setConnectionManager(connMgr)
.setMaxConnPerRoute(MAX_CONNECTIONS_TOTAL) // bad, ignored
.build();
return HttpClientBuilder.create() // good
.setMaxConnPerRoute(MAX_CONNECTIONS_TOTAL)
.setMaxConnTotal(MAX_CONNECTIONS_TOTAL)
.build();
return HttpClientBuilder.create() // good
.setConnectionManager(connMgr)
.build();Observation: Apache HttpClient uses the default timeouts.
Problem: The default timeouts of Apache HttpClient are probably too high, if not infinite.
This has impact on the stability of the app if too many threads are blocked waiting for a connection or a response.
Solution: Always set the timeouts explicitly. Use best practice values: Read/socket timeout ~4000 ms (note:
depends largely on use case and expected latency of remote calls),
Connect timeout ~250 ms, Connection Manager/Request timeout = connect timeout + slack 250+100 = ~350 ms.
Rule name: HttpClientBuilderWithoutTimeouts
Example:
RequestConfig requestConfig = RequestConfig.custom()
.setConnectionRequestTimeout(350)
.setConnectTimeout(250)
.setSocketTimeout(4000)
.build(); // good, all timeouts set
return HttpClientBuilder.create()
.setDefaultRequestConfig(requestConfig) // good, all timeouts set, if missing > default timeouts?
.build();
return HttpClientBuilder.create() // bad, no default HttpClient set with explicit timeouts set
.setConnectionTimeToLive(180, TimeUnit.SECONDS)
.build();Observation: Netflix Hystrix is used for e.g. circuit breaker.
Problem: Hystrix is not actively maintained anymore.
Solution: Netflix recommends to use open source alternatives like resilience4j.
Rule name: AvoidDeprecatedHystrix
See: DZone article on Hystrix alternatives
and resilience4j on GitHub
Observation: An HttpClient is created and combined with request-response.
Problem: Apache HttpClient with its connection pool and timeouts should be setup once and then used for many requests.
It is quite expensive to create and can only provide the benefits of pooling when reused in all requests for that connection.
Solution: Create/build HttpClient with proper connection pooling and timeouts once, and then use it for requests.
Rule name: AvoidRecreatingHttpClient
Example:
ResponseEntity<Object> connectBad(Object req) {
HttpEntity<Object> requestEntity = new HttpEntity<>(req);
HttpClient httpClient = HttpClientBuilder.create().setMaxConnPerRoute(10).build(); // bad
return remoteCall(httpClient, requestEntity);
}Observation: A Retry mechanism is used in more than one location in a call chain. This can be by Resilience4j Retry or @Retry, or Spring @Retryable.
Problem: Multiple Retry locations in a call chain multiply the number of calls. For 2x retry on 3 locations (service calls) in a chain calling a system which is just recovering,
results in 3 x 3 x 3 = 27 calls instead of 1. This may cause it not being able to restart.
.
Solution: Have the retry mechanism in one location in the chain only, recommended only the one closest to the user.
This could be achieved with passing a token/header which indicates whether retrying is already managed.
Rule name: RetryCanCauseOverload
See: AWS Timeouts, retries, and backoff with jitter
Example:
import io.github.resilience4j.retry.annotation.Retry;
@Retry(name = "some-service") // inform
public class Foo {
public Response callSomeService() {
//...and someService does a Retry for a call to the next service
}
}Observation: When troubleshooting Reactor, Blockhound can be used to find blocking calls. It needs proper stack traces which can be achieved by Hooks.onOperatorDebug().
Problem: Retaining full stack traces by reactor.core.publisher.Hooks.onOperatorDebug() uses StackWalker, this can have much CPU overhead.
Solution: Remove Hooks.onOperatorDebug() when not debugging Reactor for blocking calls.
Note: Enabling the system property reactor.trace.operatorStacktrace (default=false) is another way to retain the stack traces with the same problem.
Rule name: AvoidReactorDebugOverhead
See: 1. JDK 11 performance problem and 2. 100% CPU usage of Log4j2
Example:
import reactor.core.publisher.Hooks;
public class Foo {
public void bar() {
Hooks.onOperatorDebug(); //bad
}
}Observation: For Apache HttpComponentsClientHttpRequestFactory, both the constructor which takes a HttpClient and setHttpClient method are called.
Problem: If you use both on the same factory, you discard all configuration done on the one provided in the constructor because it is replaced by the one provided to the setter.
Solution: Don't use both on the same factory, provide the HttpClient only once to the factory.
Rule name: AvoidDiscardingHttpClientConfig
Example:
class Bad {
ClientHttpRequestFactory getFactory(HttpClientConfiguration config) {
HttpComponentsClientHttpRequestFactory factory = new HttpComponentsClientHttpRequestFactory(HttpClientBuilder.create()
.setMaxConnTotal(config.getMaxTotalConnections())
.setMaxConnPerRoute(config.getMaxConnPerRoute())
.build());
factory.setHttpClient(createHttpClient(config)); //bad
return factory;
}
}
class Good {
ClientHttpRequestFactory getFactory(HttpClientConfiguration config) {
HttpComponentsClientHttpRequestFactory factory =
new HttpComponentsClientHttpRequestFactory(createFullyConfiguredHttpClient(config));
return factory;
}
}Observation: HttpHost constructor with only one argument is used, meant just for hostname.
Problem: the HttpHost constructor with one argument must only be provided with a host name, the default port 80 and protocol http are implied.
The mistake of providing a URL and assuming it will be parsed into hostname, port and protocol is easily made.
When this HttpHost is then used for a route and stored socketConfig for, port 80 is added for the host and the socketConfig is stored with the wrong key and will not be used.
It is typically difficult to find out if the config is actually used. Note that http-client-monitor helps here.
Solution: Use the HttpHost constructor with 2 (including port) or preferably 3 arguments (including port and protocol).
Rule name: AvoidHttpHostOneArgumentConstructor
Example:
import org.apache.http.HttpHost;
class Foo {
private static final String URL = "localhost:8080";
private static final HttpHost hostBad1 = new HttpHost("localhost:8080"); // bad
void bar() {
HttpHost hostBad2 = new HttpHost(URL);//bad
HttpHost hostGood1 = new HttpHost("localhost", 8080, "http"); //good
}
}Observation: Spring RestTemplate uses a ClientHttpRequestFactorySupplier as a @Bean to configure connection pooling and timeouts.
Problem: The org.springframework.boot.web.client.ClientHttpRequestFactorySupplier may return a HttpComponentsClientHttpRequestFactory which you supply as @Bean, however,
this can silently go wrong and e.g. an unconfigured SimpleClientHttpRequestFactory can be returned.
Default pool size and timeouts will be used, possibly resulting in very slow connection use.
Solution: Provide you own supplier with explicit pool sizing and timeouts by a class implementing Supplier.
Rule name: AvoidClientHttpRequestFactorySupplier
Example:
import org.springframework.boot.web.client.ClientHttpRequestFactorySupplier;
import org.springframework.web.client.RestTemplate;
class Bad {
void bad() {
RestTemplate restTemplate = new RestTemplateBuilder(rt -> rt.getInterceptors()
.add((request, body, execution) -> {
request.getHeaders().add("SomeKey", someKey);
return execution.execute(request, body);
}))
.requestFactory(new ClientHttpRequestFactorySupplier()) // bad
.uriTemplateHandler(defaultUriBuilderFactory)
.build();
return restTemplate;
}
}
class MyClientHttpRequestFactorySupplier implements Supplier<ClientHttpRequestFactory> {
public ClientHttpRequestFactory get() {
PoolingHttpClientConnectionManager poolingHttpClientConnectionManager = new PoolingHttpClientConnectionManager();
poolingHttpClientConnectionManager.setDefaultMaxPerRoute(MAX_CONN_PER_ROUTE);
poolingHttpClientConnectionManager.setMaxTotal(MAX_CONN_TOTAL);
CloseableHttpClient httpClient = HttpClients.custom()
.setConnectionManager(poolingHttpClientConnectionManager)
.disableConnectionState()
.build();
return new HttpComponentsClientHttpRequestFactory(httpClient);
}
}and use it to replace the bad line in Bad example:
.requestFactory(new MyClientHttpRequestFactorySupplier()) // good
Observation: Spring BufferingClientHttpRequestFactory is used.
Problem: org.springframework.http.client.BufferingClientHttpRequestFactory buffers all incoming and outgoing streams fully in memory which may result in high memory usage.
Solution: Avoid multiple reads of the response body so it is not needed.
Rule name: BufferingClientHttpRequestFactoryIsMemoryGreedy
Example:
import org.springframework.http.client.*;
import org.springframework.web.client.RestTemplate;
public class Foo {
public RestTemplate createMemoryGreedyRestTemplate(HttpClientConfiguration httpClientConfiguration) {
ClientHttpRequestFactory factory = getClientHttpRequestFactory(httpClientConfiguration);
return new RestTemplate(new BufferingClientHttpRequestFactory(factory)); // bad
}
public RestTemplate createStreamTroughRestTemplate(HttpClientConfiguration httpClientConfiguration) {
ClientHttpRequestFactory factory = getClientHttpRequestFactory(httpClientConfiguration);
return new RestTemplate(factory); // good
}
}Observation: A primitive variable identifier or @Value member ends with time, duration or similar: time unit is missing.
Problem: Time unit like hours, seconds, milliseconds is not specified and may be assumed differently by readers.
Different assumptions will lead to errors or hidden problems like ineffective caches.
Solution: Specify the time unit in the identifier, like connectTimeoutMillis.
Rule name: AvoidTimeUnitConfusion
Example:
class RetrieveCache {
@Autowired
public RetrieveCache(final @Value("${cache.expiryTime}") long timeToLive) { // 2x bad
}
@Autowired
public RetrieveCache(final @Value("${cache.expiryTimeMillis}") long timeToLiveMillis){ // 2x good
}
}Observation: Apache HttpClient RequestConfig connectionRequestTimeout and connectTimeout have values which are typically too high.
Problem: org.apache.http.client.config.RequestConfig is used with connectionRequestTimeout and connectTimeout values > 500 milli seconds.
- connectTimeout is for establishing a (secure) connection which should be quick, say below 200 ms.
- connectionRequestTimeout is for requesting a connection from the connection manager, which should be almost as quick, say below 250 ms.
If timeouts are long, requests will wait long for an unavailable service and cause high thread usage and possibly overload.
Solution: Set connectTimeout and connectionRequestTimeout to values based on network tests, for instance 200 ms and 250 ms. respectively.
Rule name: HttpClientImproperConnectionTimeouts
Example:
import org.apache.http.client.config.RequestConfig;
public class HttpClientStuff {
private static final int CONNECTION_TIMEOUTMILLIS = 1000; // bad // timeout until a connection is established
private static final int CONNECTIONREQUEST_TIMEOUTMILLIS = 5000; // bad // timeout when requesting a connection from the connection manager
private static final int SOCKET_TIMEOUTMILLIS = 5000; // timeout of waiting for data
public RequestConfig requestConfigWithTimeouts() {
RequestConfig requestConfig = RequestConfig.custom()
.setConnectionRequestTimeout(CONNECTIONREQUEST_TIMEOUTMILLIS)
.setConnectTimeout(CONNECTION_TIMEOUTMILLIS)
.setSocketTimeout(SOCKET_TIMEOUTMILLIS)
.build();
return requestConfig;
}
}Observation: The default http client of Feign is used, which is java.net.HttpURLConnection.
Problem: java.net.HttpURLConnection does not reuse connections when using (mutual) TLS, this causes connection handshake overhead: extra CPU and latency.
Solution: Use an http client with Feign that has connection reuse for (mutual) TLS connection. For example, use Apache HttpClient 4 with proper connection pool settings (see IBI03, IBI07 and IBI10).
Rule name: DefaultFeignClientWithoutTLSConnectionReuse
Example:
class FeignConfig {
@Bean
public Feign.Builder feignBuilderBad() {
Client feignClient = new Client.Default(setupSSLContextForMutualTLS().getSocketFactory(), new DefaultHostnameVerifier()); // bad
return Feign.builder().retryer(Retryer.NEVER_RETRY).client(feignClient);
}
@Bean
public Feign.Builder feignBuilderGood(CloseableHttpClient client) {
Client feignClient = new ApacheHttpClient(client); // good: wrap Apache HttpClient with connection pool enabled
return Feign.builder().retryer(Retryer.NEVER_RETRY).client(feignClient);
}
}Observation: Connection configuration, that is, timeouts and pool sizes, are hardcoded.
Problem: configuring connection settings like timeouts and pool sizes in code (int values) makes it difficult to manage and tune these settings.
Solution: use property files, e.g. yml, to define the values for these settings for each called service.
Rule name: AvoidHardcodedConnectionConfig
Example:
class AvoidHardcodedConnectionConfig {
private static final int CONNECTION_TIMEOUT = 50000;// bad
private static final int SOCKET_TIMEOUT = 250000;// bad
private static final int TRANSACTION_TIMEOUT_SECONDS = 2;// bad
private static final int CONNECTION_TIMEOUTMILLIS = 1000;// bad
private static final int MAX_CONN_PER_ROUTE = 10;// bad
static final int MAX_PER_ROUTE = 100;// bad
final int MAX_NUM_ROUTES = 2; // bad
int some_timeout_ms = 300; //good, variable
private static final String LOGIN_TIMEOUT_PROPERTY_KEY = "loginTimeout"; // good, not an int
final int other_timeout_s_blank; // bad, blank final and hard coded
private final int connectTimeoutMillis; // good, not hard coded
AvoidHardcodedConnectionConfig(@Value("${connect_timeout:1000}") int connectTimeoutMillis) {
this.connectTimeoutMillis = connectTimeoutMillis; // good, not hard coded
other_timeout_s_blank = 5; // field bad
final int timeout_s_local = 4; // good, method level
int otherTimeoutInS = 3; // good, var
}
}Observation: SAAJ SOAP messaging is used.
Problem: SAAJ uses DOM to load the XML document in memory which uses a TransformerFactory. The implementation class of it is loaded on every call which is expensive and causes lock contention under load. This means long response times.
Solutions:
- Use Axiom SOAP messaging which uses the faster StAX instead of DOM. We did not encounter excessive classloading with Axiom.
- If you want or have to use SAAJ, set the proper JVM parameter/system property for TransformerFactory and MessageFactory (see IUOXAR09 link below) to prevent the excessive class loading.
Note: When you have solved this isssue with JVM params or like the example, yet, in another file then where the violation occurs, just suppress the rule with the reason explained, how it is solved.
Note: Unfortunately, AxiomSoapMessageFactory has been removed from spring-ws with Spring Boot 3.0. In that case, only solution 2 seems feasible.
Rule name: AvoidExcessiveClassloadingWithSaajSoap
Example:
import org.springframework.ws.soap.saaj.SaajSoapMessageFactory;
class Bad {
private final SaajSoapMessageFactory mfField; // bad, also thread-unsafe
public WebServiceTemplate getTemplate(final String uri, final HttpClientMessageSender httpClientMessageSender) throws SOAPException {
SoapMessageFactory saajSoapMessageFactory = new SaajSoapMessageFactory(MessageFactory.newInstance()); // bad
//..
}
}
class Good {
static {
System.setProperty("javax.xml.transform.TransformerFactory", "com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl"); // may be different for you
System.setProperty("javax.xml.soap.MessageFactory", "com.sun.xml.internal.messaging.saaj.soap.ver1_1.SOAPMessageFactory1_1Impl"); // may be different for you
}
public WebServiceTemplate getTemplate(final String uri, final HttpClientMessageSender httpClientMessageSender) throws SOAPException {
SoapMessageFactory saajSoapMessageFactory = new SaajSoapMessageFactory(MessageFactory.newInstance()); // good
//..
}
}See: IUOXAR09: XML related XXXFactory.newInstance() is called repeatedly.
This categry could be seen as a subcategory of the previous category. However, above mostly deals with remote connections, asynchony mostly deals with threading and parallelism. We assume asynchronous calls are typically made to remote services.
Observation: A Spring @Async thread pool has default sizes, or is not sized based on measurements or best practices
Problem: Spring @Async uses a ThreadPoolTaskExecutor. This Executor and Java's ThreadPoolExecutor have a setCorePoolSize and setMaxPoolSize method.
Spring's Executor has default core size = 1 and max size = unlimited, which are almost never appropriate, it uses too many thread resources when the called service is slow or unavailable.
Setting values to e.g. 100 is likely too high, causing too much thread resources usage and scheduling/context switching overhead.
Additionally, if remote connections use a connection pool and the number of threads is higher than the number of connections in the pool, threads will have to wait for a connection.
Setting values to 1 allows for serial execution only, while one typically wants to utilize parallelism.
Solution: The max number of threads should be based on request rate (tps) and response times of a healthy called service in peak load situations, such that all requests can be handled without queueing and no more threads are used than necessary.
Make core size N smaller than max size to reduce overhead and resource usage in the common case when N threads is enough. E.g. core size = 10, max size = 30.
Note that the pool will only grow bigger than core size when the queue limit is reached.
Example:
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
executor.setCorePoolSize(10); // based on measurements
executor.setMaxPoolSize(30); // based on measurements
executor.setQueueCapacity(10); // see next item (IA02)Observation: A Spring @Async thread pool queue has default capacity, or is not sized based on measurements or best practices
Problem: Spring @Async uses a ThreadPoolTaskExecutor. This Executor and Java's ThreadPoolExecutor have a setQueueCapacity method.
Default capacity = unlimited, which is almost never appropriate, it uses too much memory when the called service is slow or unavailable.
Setting the capacity to e.g. 100 is likely too high, causing too much response time delay.
Setting the capacity to 0 allows for no full-function handling of spike load or hiccups in the called chain, beyond max pool size utilization.
Solution: The queue capacity should be based on request rate (tps) and response times of a healthy called service in peak load situations, and to accommodate for some level of spike load and hiccups in the called chain.
Make queue capacity equal to pool core size to have at most 1 request waiting for each thread doing a service call.
Example:
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
executor.setCorePoolSize(10); // based on measurements
executor.setMaxPoolSize(30); // based on measurements
executor.setQueueCapacity(10); // based on measurementsObservation: A Spring @Async thread pool is used for more than one async remote service call
Problem: Long response times or unavailability of one remote service will eat up threads of the pool so the other service will get no threads to do the remote call.
Solution: If this is undesirable, use a separate pool for each async service call. (Bulkhead pattern)
Observation: A Spring ThreadPoolTaskExecutor is not monitored
Problem: If you don't see the usage of threads and the queue under load, you cannot size these properly.
Solution: You need to measure to know how to size.
ThreadPoolTaskExecutor can be exposed as mbean and monitored by VisualVM or other mbean browser,
or monitored by using micrometer (example in afterburner).
Make sure to give the threads a proper name, this makes them easier to recognize in thread dumps, heap dumps and profiling.
Observation: A Spring ThreadPoolTaskExecutor queue capacity is not configured
Problem: The org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor has a default queue capacity which is unlimited which can lead to an out of memory situation.
Solution: Call setQueueCapacity, for instance with a value equal to CorePoolSize.
Note that the pool will only grow beyond CorePoolSize up to MaxPoolSize when the queue is full.
Rule name: SetQueueCapacityForTaskExecutor
Example:
private ThreadPoolTaskExecutor createExecutor() {
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
executor.setCorePoolSize(10);
executor.setQueueCapacity(10); // bad if missing
executor.setMaxPoolSize(20);
executor.initialize();
return executor;
}Observation: A blocking asynchronous call future.get() without time-out is used.
Problem: Stalls indefinitely in case of hanging thread(s) and consumes a thread. Threads may get stuck in database, a remote system, because of a network hiccup, in error or other exceptional situations.
Solution: Use the version with timeout and handle a timeout situation.
Rule name: AvoidFutureGetWithoutTimeout
Example:
future.get(long timeout, TimeUnit unit)See: Monix Best Practice
Observation: A blocking future.join() is called without a timeout.
Problem: It cannot deal with hanging threads. Threads may get stuck in database, a remote system, because of a network hiccup, in error or other exceptional situations.
Solution: Provide a timeout before the join and handle the timeout. For example a future.get(timeout, unit), a orTimeout() or a completeOnTimeout(). You may want to use CompletableFuture.allOf().
Rule name: AvoidFutureJoinWithoutTimeout
Example:
class Foo {
private List<Order> getOrdersBad(List<CompletableFuture<Order>> getOrdersFutures) {
List<Order> orders = getOrdersFutures.stream()
.map(CompletableFuture::join) // bad, NO timeout provided above
.collect(Collectors.toList());
return orders;
}
private List<Order> getOrdersGood(List<CompletableFuture<Order>> getOrdersFutures) {
// added to deal with timeout
CompletableFuture<Void> allFuturesResult = CompletableFuture.allOf(getOrdersFutures.toArray(new CompletableFuture[getOrdersFutures.size()]));
try {
allFuturesResult.get(5L, TimeUnit.SECONDS); // good
} catch (Exception e) { // should make explicit Exceptions
//log error
}
List<Order> orders = getOrdersFutures.stream()
.filter(future -> future.isDone() && !future.isCompletedExceptionally()) // keep only the ones completed -- added to deal with timeout
.map(CompletableFuture::join) // good, timeout provided above
.collect(Collectors.toList());
return orders;
}
}See: completablefuture-and-timeout and completablefuture-allof-and-errors
Observation: Future.supplyAsync is used for remote calls and it uses the common pool (the default).
Problem: The common pool is meant for processing of in-memory data. The number of threads in the common pool is equal to the number of CPU's,
which is suitable to keep all CPU's busy with in-memory processing.
For I/O, however, this number is typically not suitable because relatively much time is spent waiting for the response and not in CPU.
This likely exhausts the common pool for some time thereby blocking all other use of the common pool.
The common pool must not be used for blocking calls, see Be Aware of ForkJoinPool#commonPool()
Solution: A separate, properly sized, pool of threads (an Executor) should be used for the async calls.
Rule name: AvoidCommonPoolForFutureAsync
Example:
public class Foo {
private final ExecutorService asyncPool = Executors.newFixedThreadPool(8);
void bad() {
CompletableFuture<Pair<String, Boolean>>[] futures = accounts.stream()
.map(account -> CompletableFuture.supplyAsync(() -> isAccountBlocked(account))) // bad
.toArray(CompletableFuture[]::new);
}
void good() {
CompletableFuture<Pair<String, Boolean>>[] futures = accounts.stream()
.map(account -> CompletableFuture.supplyAsync(() -> isAccountBlocked(account), asyncPool)) // good
.toArray(CompletableFuture[]::new);
}
}Observation: parallelStream which uses the ForkJoinPool::commonPool is used for blocking (I/O, remote) calls.
Problem: The common pool is meant for processing of in-memory data. The number of threads in the common pool is equal to the number of CPU's - 1,
which is suitable to keep all CPU's busy with in-memory processing.
For I/O or other blocking calls, however, this number is typically not suitable because relatively much time is spent waiting for the response and not in CPU.
This likely exhausts the common pool for some time thereby blocking all other use of the common pool.
The common pool must not be used for blocking calls, see Be Aware of ForkJoinPool#commonPool()
Solution: A separate, properly sized, pool of threads (an Executor or ForkJoinPool) should be used for the async calls.
Rule name: AvoidCommonPoolForBlockingCalls
Example:
public class Foo {
final List<String> list = new ArrayList();
final ForkJoinPool myFjPool = new ForkJoinPool(10);
final ExecutorService myExePool = Executors.newFixedThreadPool(10);
void bad1() {
list.parallelStream().forEach(elem -> storeDataRemoteCall(elem));
}
void good1() {
CompletableFuture[] futures = list.stream().map(elem -> CompletableFuture.supplyAsync(() -> storeDataRemoteCall(elem), myExePool))
.toArray(CompletableFuture[]::new);
CompletableFuture.allOf(futures).get(10, TimeUnit.MILLISECONDS);
}
void good2() throws ExecutionException, InterruptedException {
myFjPool.submit(() ->
list.parallelStream().forEach(elem -> storeDataRemoteCall(elem))
).get();
}
String storeDataRemoteCall(String elem) {
// do remote call, blocking. We don't use the returned value.
RestTemplate tmpl;
return "";
}
}See for the example good2: custom-thread-pool-in-parallel-stream.
Observation: An Axual (Kafka) producer is created for each method call.
Problem: each Producer takes threads and memory. If you create it in each method call, and call this frequently, it will result in an explosion of threads and memory used and lead to Out Of Memory Error.
Solution: Since the Axual Producer is thread-safe, it should be shared e.g. from a static field. Note that the AxualClient is not thread-safe.
Rule name: AxualProducerCreatedForEachMethodCall
Example:
import io.axual.client.producer.Producer;
public class AxualProducerBad {
public void publishToEventStream() {
Producer<String, String> producer = axualClient.buildProducer(producerConfig); // bad
producer.produce(msg);
}
}
class AxualProducerGood1{
private static final Producer<String, String> producer = AxualClient.buildProducer(producerConfig);
}
@Configuration
class AxualProducerGood2{
public Producer<String, String> axualProducer() {
Producer<String, String> producer = axualClient.buildProducer(producerConfig);
return producer;
}
}Observation: parallelStream which uses the ForkJoinPool::commonPool is used.
Problem: Collection.parallelStream() and .stream().parallel() use the common pool, with #threads = #CPUs - 1.
It is designed to distribute much CPU work over the cores.
It is not meant for remote calls nor other blocking calls.
In addition, parallelizing has overhead and risks, should only be used for much pure CPU processing.
The common pool must not be used for blocking calls, see Be Aware of ForkJoinPool#commonPool()
Solution: For remote/blocking calls: Use a dedicated thread pool with enough threads to get proper parallelism independent of the number of cores.
For pure CPU processing: use ordinary sequential streaming unless the work takes more than about 0,1 ms in sequential form and proves to be faster with parallelization.
So only for large collections and much processing without having to wait.
Rule name: AvoidParallelStreamWithCommonPool
Example:
import java.util.*;
import java.util.concurrent.*;
import java.util.stream.*;
public class Foo {
final Map<String, String> map = new HashMap();
final List<String> list = new ArrayList();
final List<String> hugeList = new ArrayList(); //1000+ elements
final ForkJoinPool myFjPool = new ForkJoinPool(10);
final ExecutorService myExePool = Executors.newFixedThreadPool(10);
void bad1() {
list.parallelStream().forEach(elem -> someCall(elem)); //bad
}
void bad2() {
map.entrySet().parallel().stream().forEach(entry -> someCall(entry.getValue())); //bad
}
void exceptionalProperUse() {
hugeList.parallelStream().forEach(elem -> heavyCalculations(elem)); //flagged but may be good, should suppress when proven to be faster than sequential form
}
void good1() {
CompletableFuture[] futures = list.stream().map(elem -> CompletableFuture.supplyAsync(() -> someCall(elem), myExePool))
.toArray(CompletableFuture[]::new);
CompletableFuture.allOf(futures).get(3, TimeUnit.SECONDS);
}
void good2() throws ExecutionException, InterruptedException {
myFjPool.submit(() ->
list.parallelStream().forEach(elem -> someCall(elem))
).get();
}
String someCall(String elem) {
// do some call, don't know if remote or blocking. We don't use the returned value.
return "";
}
String heavyCalculations(String elem) {
// calculate a lot
return "";
}
}See for the example good2: custom-thread-pool-in-parallel-stream.
Observation: Project Reactor Flux.parallel().runOn() is used.
Problem: The data is divided on a number of 'rails' matching the number of CPU cores (by default).
This is only the proper solution in case much CPU processing is performed: if the sequential form of the CPU work takes more than 0,1 ms of CPU time.
With remote calls this is usually not the case. In addition, it introduces more complexity with risk of failures.
Solution: Remove parallel().runOn. For pure CPU processing: only use parallel().runOn() when the work takes more than about 0,1 ms in sequential form and proves to be faster with parallelization.
So only for large collections and much processing.
Rule name: AvoidParallelFlux
Note: If the call you do is a blocking (remote) call, you are not fully reactive. Then you still need a thread pool for threads waiting for the response.
This can be achieved by wrapping the blocking call, using flatMap and subscribeOn or publishOn.
See:
- Project Reactor Schedulers
- ParallelFlux for CPU work
- How to wrap a blocking call
- ParallelFlux vs flatMap() for a Blocking I/O task
import reactor.core.publisher.*;
class FooBad {
public Flux<Account> getResponseAccounts(List<AccountKey> accountKeys, List<FieldName> requestedFields) {
return Flux.fromIterable(accountKeys)
.parallel(schedulerProps.getParallelism()) //bad
.runOn(scheduler)
.flatMap(accountKey -> constructAccountDetails(accountKey, requestedFields))
.sequential();
}
}
class FooGood_NonBlocking {
public Flux<Account> getResponseAccounts(List<AccountKey> accountKeys, List<FieldName> requestedFields) {
return Flux.fromIterable(accountKeys)
.flatMap(accountKey -> constructAccountDetails(accountKey, requestedFields));
// for a non-blocking/async call, fully reactive
}
}
class FooGood_Blocking {
public Flux<Account> getResponseAccounts(List<AccountKey> accountKeys, List<FieldName> requestedFields) {
return Flux.fromIterable(accountKeys)
.flatMap(accountKey -> Mono.fromCallable(() -> {constructAccountDetails(accountKey, requestedFields); return accountKey;})
.subscribeOn(Schedulers.boundedElastic()), schedulerProps.getParallelism());
// for a blocking (I/O) call you still need a thread pool
}
}Observation: Mapped Diagnostic Context (MDC) is used in projector Reactor flows.
Problem: Mapped Diagnostic Context (MDC) of logging frameworks uses ThreadLocals to store things like traceIds from headers, userId, correlationId.
Reactive programming uses multiple threads to handle a request, and one thread can deal with asynchronous steps of many requests.
Therefore, MDC is tricky to use in reactive context and may take much processing time to propagate to other threads, likely so for much data in the MDC.
Solution: Propagate by use of deferContextual and use directly from the Context only when and where needed.
Avoid io.micrometer:context-propagation default as well as automatic mode, as both have an impact on performance.
Rule name: AvoidMDCInReactor
See: GitHub issue, search for 'performance'; and reactor Context.
Example:
import reactor.core.publisher.*;
import reactor.util.context.Context;
import org.slf4j.MDC;
import net.logstash.logback.argument.StructuredArguments;
class FooBad {
public Flux<ServerResponse> doIt(Map<String,String> contextMap) {
return Flux.fromIterable(service.doWork())
.doOnNext(s -> MDC.setContextMap(contextMap))
.doOnNext(
response -> {
log.info("your log"); // includes trace ids from MDC, put in a ServletFilter, specified by log configuration
});
}
}
class FooGood {
public Flux<ServerResponse> doIt() {
// using deferContextual gives access to the read-only ContextView created in the ContextFilter
return Flux.deferContextual(contextView -> service.doWork()
.doOnNext(response -> {
log.info("your log", StructuredArguments.entries((Map)contextView));
}));
}
}Observation: Reactor Hooks.onEachOperator is used.
Problem: Using Reactor Hooks.onEachOperator means executing the code on every operator in the Reactor flow, for every element.
This typically means much processing time. So, for example copying a context Map on every operator (for MDC propagation) is not a good idea.
Solution: Just do processing when and where actually needed. For example, for MDC propagation this is described as the manual strategy of Spring Cloud Sleuth. E.g. get userId from the Reactor Context when needed to log.
Rule name: AvoidReactorOnEachOperator
See: context-propagation-with-project-reactor-2-the-bumpy-road-of-spring-cloud, and reactor Context.
Example:
import reactor.core.publisher.*;
// class to propagate MDC to other thread on each flow operation
@Configuration
public class FooBad {
private String MDC_CONTEXT_REACTOR_KEY = "CONTEXT";
@PostConstruct
private void contextOperatorHook() {
Hooks.onEachOperator(MDC_CONTEXT_REACTOR_KEY,
Operators.lift((scannable, coreSubscriber) -> new MdcContextLifter<>(coreSubscriber))
);
}
}
// In the following way, MDC propagation is not needed
class FooGood {
public Flux<ServerResponse> doIt() {
// using deferContextual gives access to the read-only ContextView created in the ContextFilter
return Flux.deferContextual(contextView -> service.doWork()
.doOnNext(response -> {
log.info("your log", StructuredArguments.entries((Map)contextView));
}));
}
}Observation: Avoid ExecutorService invokeAll and invokeAny without timeout.
Problem: Stalls indefinitely in case of stalled Callable(s) and consumes threads.
Solution: Provide a timeout to the invokeAll/invokeAny method and handle the timeout.
Rule name: AvoidExecutorInvokeWithoutTimeout
Note: Also in case all Callables have a proper timeout, use an overall timeout on the ExecutorService level, e.g. for issues with handling of the individual Callable timeouts.
Example:
import java.util.concurrent.*;
class Foo {
private List<Future<ServiceResult>> executeTasksBad(Collection<Callable<ServiceResult>> tasks, ExecutorService executor) throws Exception {
return executor.invokeAll(tasks); // bad, no timeout
}
private List<Future<ServiceResult>> executeTasksGood(Collection<Callable<ServiceResult>> tasks, ExecutorService executor) throws Exception {
return executor.invokeAll(tasks, OUR_TIMEOUT_IN_MILLIS, TimeUnit.MILLISECONDS); // good
}
}See our presentation on Java Performance Pitfall: improper caching.
Observation: Default cache key generation with toString is used. DefaultServiceCacheConfiguration / DefaultCacheKeyGenerator defines the key used for caching as the class name with concatenated the toString() called on all method arguments.
Problems:
- This is a problem especially for non-String arguments
- toString method is meant to return a human readable representation of the object, not a part of a cache key.
- toString may not be defined on the argument and return the class name and a hash for its location on the heap, thereby returning the object identity in stead of equality. This typically results in no cache hits, only misses.
- For a cache in the database: The resulting key may be too large to fit in the database column.
- It is unclear from the source code what the key is and why it is defined like it is, the motivation. Proper javadoc or other cache key description is typically missing.
- This default cache key definition is unclear, fragile and error prone.
Solutions:
- Make all cache keys explicit and specific. Do not use the default keys.
- Do not use toString for a cache key, instead create an explicit, separate method to generate (part of) the cache key, named for example getKey:
public String getKey() {
return userId;
}- Explain design decisions and conditions around the key definition in a document and/or in javadoc.
- The DefaultCacheKeyGenerator must not be used anymore and should be deprecated. To be changed in ServiceCache.
- An efficient implementation would be a CompositeKey composed of all arguments to use as key in Ehcache Element:
public class CompositeKey {
private String userKey;
private String siteKey;
public CompositeKey(User user, Site site) {
userKey = user.getKey();
siteKey = site.getKey();
}
public boolean equals(Object other) {
return (other != null && other instanceof CompositeKey
&& ((CompositeKey)other).userKey.equals(this.userKey)
&& ((CompositeKey)other).siteKey.equals(this.siteKey));
}
public int hashCode() {
// similar to equals, using hashCode
}
}- This is efficient because it prevents String concatenation of the arguments when accessing the cache.
- Actually Spring's SimpleKey is a more generic solution, see IC13.
Rule name: partly implemented: AvoidDefaultCacheKeyGenerator.
Observation: Caching takes place in the database.
Problem: ServiceCache uses by default caching in the database which is inferior to caching in memory. A database round trip for a SELECT may take 10-50 ms. A value not yet in the cache needs 3 update/inserts and may take 100+ ms, and even more with high load.
Solution: Cache in memory with ehcache. The total amount of cache data per JVM should be limited to a couple of 100 MBs, more needed can be overflowed to disk. It could be a good idea to create a solution on application level, such that central monitoring and management can take place, e.g. for monitoring size and hit ratio and clearing caches.
Observation: Some retrieve services are not cached.
Problem: Active “F5 users” or DoS attacks could overload the back-end systems.
Solution: All get services should be cached at least for a short time, to prevent overloading of back-end systems.
Observation: Freemarker template caching is set at the default of 5 seconds.
Problem: Every 5 seconds, for every template file, the application hits the file system to find out if there is a new version of the template file. Since these don’t change in production, this is unnecessary load on the system.
Solution: Increase the time to live for the freemarker template cache to a reasonable, much higher value, possibly indefinite. Project has configured the following to be in file initiate-direct-debit-portlet.xml:
<property name="freemarkerSettings">
<props>
<prop key="template_update_delay">2500000</prop>
</props>
</property>Note that this delay is specified in seconds.
Or via configuration (e.g. in Spring):
freemarker.template.Configuration.setTemplateUpdateDelayMilliseconds(2_500_000_000L);Observation: Cache configuration is not motivated.
Problem: Circumstances may change and make the cache configuration invalid, to be updated.
Solution: Add a motivation for each configuration value. For example:
<cache maxEntriesLocalHeap="1" /> <!-- only one entry needed for all countries in one ArrayList-->
<cache timeToLiveSeconds="7200" /> <!-- business: change to country table needs to be live within 2 hours-->Observation: Cache elements are mutable.
Problem: A cache can be used by multiple threads, if elements are mutated in a thread-unsafe manner, concurrency issue will occur. Some caches store elements by-reference and others by-value, if cache implementation is changed, application behavior may change because of this.
Solution: Make cache elements immutable/unmodifiable
Observation: Cache elements are large domain objects.
Problem: Caches are meant to temporarily contain data / values and are separate from your application logic / behavior.
Solution: Cache Serializable value objects used by your domain objects.
Observation: Cache properties are not monitorable.
Problem: If we cannot see if the cache has hits, if the hit ratio is like we expect, we don't know if the cache is useful at all.
Solution: Make all caches monitorable by exposing them as MBeans. Name the CacheManager. Ehcache provides those mbeans with statistics. Like:
cacheManager.setName("Mobile App CacheManager");
...
final MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer();
ManagementService.registerMBeans(cacheManager, mbeanServer, true, true, true, true);
LOG.info("MBean registration is done.");In Spring config:
<beans>
<bean id="exporter" class="org.springframework.jmx.export.MBeanExporter" lazy-init="false">
<property name="beans">
<map>
<entry key="bean:name=testBean1" value-ref="testBean"/>
</map>
</property>
</bean>
<bean id="testBean" class="org.springframework.jmx.JmxTestBean">
<property name="name" value="TEST"/>
<property name="age" value="100"/>
</bean>
</beans>Observation: Cache keys are unnecessarily long. For instance each key starts with com.company.department.subdepartment.tribe.topic.
Problem: All keys together take up a substantial part of the heap. Often the keys take up more space than the values.
Solution: Make all keys just long enough to be unique, do not have a part identical for all keys. Do not include the fully qualified class names.
Observation: Cache keys are sums of hash codes of method arguments.
Problem: Hash codes are int-s which have a non-negligible chance to be equal for unequal objects. If two keys are unintentionally equal, session data will be mixed up.
Solution: Never use keys based on hashCode-s.
Observation: ehcache default cache configuration is used.
Problem: Default configuration is confusing: it only applies to programmatically created caches by addCache(). These caches can be created and used without explicit configuration, yet implicitly with the default configuration, which can be wrong. Caches need to be tuned individually.
Solution: Remove the default cache configuration. This way, an error is generated if caches are created by name (programmatically) with no defaultCache loaded. This is clear and fail-fast. Configure each ehcache declaratively in ehcache.xml.
Observation: a remote (cloud) cache like Redis is used without a local cache for elements typically accessed more than once, e.g. for static-ish data like countries, currencies
Problem: every access to the static data will take a remote call to the remote cache, serialization of data and possibly processing to an intermediate object like a Map to lookup e.g. a country. This means extra time is added the response time for the user.
Solution: Add a local, in process, by reference cache for high-performance; in addition to a remote cache. Ehcache is the most used local, in-process cache. Add a local cache in case of high load and you expect a significant hit ratio (say >20%) on the local cache. The redis cache enables scaling up of the calling service, it reduces load on the back-end and the local cache reduces load on the redis cache.
Observation: A non-overridden Object.toString may be called on a spring KeyGenerator.generate method parameter.
Problem: The non-overridden Object.toString returns a String representing the identity of the object. Because this is different for two objects with the same value, cache keys will be different and the cache will only have misses and no hits.
Solution: return a SimpleKey composed of method and the params. Note: equals and hashCode must be properly implemented for each param.
Rule name: AvoidIdentityCacheKeys
Example:
import org.springframework.cache.interceptor.KeyGenerator;
import org.springframework.util.StringUtils;
import org.springframework.cache.interceptor.SimpleKey;
class Bad implements KeyGenerator {
public Object generate(Object target, Method method, Object... params) {
List<Object> objArray = Arrays.asList(params);
return target.getClass().getName() + "_" + method.getName() + "_"
+ StringUtils.arrayToDelimitedString(params, "_"); // bad, do not concatenate without casting
}
}
class NotBad implements KeyGenerator {
public Object generate(Object target, Method method, Object... params) {
if (params.length != 1) {
throw new IllegalArgumentException("KeyGenerator for GetProfileCache assumes 1 parameter 'profileId', found: " + params);
}
String profileId = (String)params[0];
StringJoiner joiner = new StringJoiner("_");
joiner.add(target.getClass().getSimpleName()).add(method.getName()).add(profileId);
return joiner.toString();
}
}
class Good implements KeyGenerator {
@NotNull @Override
public Object generate(Object target, Method method, Object... params) {
return new SimpleKey(method, params);
}
}In the above, method is included to ensure to get different keys if this KeyGenerator is used on different classes/methods. The Method object implements equals and hashCode properly.
Observation: Default key generation is used with @Cacheable, no KeyGenerator is used.
Problem: With default key generation, an object of Spring's SimpleKey class is used and its value is composed of just the method parameter(s). It does not include the method (nor class), which is unclear and risky, it may cause cache data mix-up.
Solution: Create a KeyGenerator and make it generate a unique key for the cache per cached value, by use of SimpleKey composed of method and the appropriate method parameters.
Rule name: UseExplicitKeyGeneratorForCacheable
See: Spring 4.0 Caching
Example:
import org.springframework.cache.annotation.Cacheable;
class Foo {
@Cacheable(cacheNames = {"DATA"}, sync = true, keyGenerator = "cacheKeyGenerator")
public Object getDataGood(String id) {
return fetchFromBackend(id);
}
@Cacheable(value="DATA", sync = true) // bad, keyGenerator missing
public Object getDataBad(String id) {
return fetchFromBackend(id);
}
}Observation: Spring's SimpleKey creation for a cache lacks either the method or the method parameters.
Problem: Both method and the method parameters are needed to make a unique key for a cache entry.
If one is missing, a key may be the same for different methods or different parameter values, which may cause cache data mix-up.
Solution: Create a SimpleKey composed of both the method object and the params Object[].
Rule name: AvoidSimpleKeyCollisions
See: Spring 4.0 Caching
Example:
import org.springframework.cache.interceptor.*;
import java.lang.reflect.Method;
class BadCacheKeyGenerator implements KeyGenerator {
@Override
public Object generate(Object target, Method method, Object... params) {
return new SimpleKey(params); // bad
}
}
class GoodCacheKeyGenerator implements KeyGenerator {
@Override
public Object generate(Object target, Method method, Object... params) {
return new SimpleKey(method, params); // good
}
}
class AlsoGoodCacheKeyGenerator implements KeyGenerator {
@Override
public Object generate(Object target, Method method, Object... params) {
return SimpleKeyGenerator.generateKey(method, params); // good
}
}Observation: For a @Cachable (with keyGenerator) annotated method, the parameters that make up the cache key do not properly implement the required methods.
Problem: When
- concatenating or joining parameters in a KeyGenerator: they need to properly implement toString().
- using SimpleKey (recommended): the parameters need to properly implement equals() and hashCode(). Failing to do so may lead to caching data mix-up.
Solution: Create a SimpleKey composed of both the method object and the params Object[] and make sure the params properly implement equals and hashCode.
Rule name: EnsureProperCacheableParams
Note: This rule is just informational, because it cannot actually check if it is implemented correctly or not.
See: Spring 4.0 Caching
Example:
import org.springframework.cache.annotation.Cacheable;
import java.time.*;
class Foo {
@Cacheable(value = "myCache", keyGenerator = "myGenerator")
public String getDataGood(String str, LocalDate date) {
return service.getData(input);
}
@Cacheable(value = "myCache", keyGenerator = "myGenerator")
public String getDataInform(MyObject input, String str, LocalDate date) { // inform
return service.getData(input);
}
}
class MyObject {
String field;
// equals, hashCode, toString missing
}Observation: The class implementing Spring's KeyGenerator uses a generic name: CacheKeyGenerator.
Problem: It is unclear where this KeyGenerator should be used, for which cache and/or for which methods.
If used on the wrong caches or methods, it may lead to cache key mix-up and user data mix-up.
Solution: Make the name specific so that it is clear where to apply this KeyGenerator in @Cacheable.
Rule name: UseClearKeyGeneratorName
Example:
import org.springframework.cache.interceptor.KeyGenerator;
public class CacheKeyGenerator // bad, unclear name
implements KeyGenerator {
public Object generate(Object target, Method method, Object... params) {
// build key and return it
}
}The application often puts much data in the user session to keep state at the server side. The HTTP and Portlet sessions have a timeout of default 30 minutes, so if the user does not sign out explicitly, data keeps occupying heap space for a long time. Heap space is limited and shared by all sessions of all portlets on the JVM, so this data limits scalability. Additionally, garbage collection pause times increase with increasing heap usage. Since this application has to deal with many concurrent users, session data should be minimized. Portlets can set portlet session attributes explicitly, or Spring can put data in the session implicitly.
Observation: Attributes are not removed from the session. They are set in the session like:
actionRequest.getPortletSession().setAttribute(DOWNLOAD_INFORMATION_FORM, downloadInformationForm);and they are never removed or not removed in all flows, being happy or unhappy.
Problem: Attributes unnecessarily occupy heap space.
Solution: Remove attributes from the session if not really needed, and as soon as possible in all controllable flows, including for instance technical exception cases, like as follows.
actionRequest.getPortletSession().removeAttribute(DOWNLOAD_INFORMATION_FORM);Spring has a PortletUtils to facilitate session attribute usage. It however does not provide a remove method. Removing can be achieved as in:
PortletUtils.setSessionAttribute(request, SESSION_ATTRIBUTE_NAME, null);Spring will than invoke removeAttribute on the portlet session.
It might also be an option to use render parameters instead, for example, in the action method:
response.setRenderParameter("page", "initiatePayment");Rule name: Available. Assumes removeAttribute occurs in same class as setAttribute. Also deals with Spring PortletUtils.
Observation: Attributes contain too much data. This is usually observed in a heap usage analysis. MemorySession objects of 5-20 kB are often observed.
Problem: If they occupy more than 1 kB, we consider this as being too much. For motivation of the norm of 1 kB: at peak load there can be like 20.000 sessions times 25 portlets = 500.000 portlet sessions, times 1 kB = 500 MB which is really a maximum.
Solution:
- Remove attributes from the session if not really needed.
- Remove attributes earlier in flows.
- Reduce the amount of data in attributes: only store what is really needed.
- Store the data more efficiently, see [TMSU05]
Observation: Session attributes are accessed from various places.
Problem: Often, attribute names are inconsistent, access handling is inconsistent and/or a method clearSession only clear a few in stead of the all session attributes. Confusing, error prone, violates the DRY principle.
Solution: Centralize and encapsulate session access in one class, where all attribute names are defined.
Observation: Attributes gotten from the session are not checked for null.
Problem: Out of sequence requests, e.g. refresh or misuse, should not result in a NullPointerException.
Solution: Centralize session access in one class and check for null. Throw an IllegalStateException to fail fast on misuse and handle with a proper message to the user.
Observation: Attributes are needed in the session, but they contain too much data.
Problem: Sessions occupy too much heap.
Solution:
- Remove collections dynamic growth overhead.
- For instance, an ArrayList has the default initial capacity of 10 elements. This memory overhead can be avoided with use of the constructor with capacity argument or removed with trimToSize().
- A HashMap can be pre-sized with initial capacity and load factor, however it has more space overhead than ArrayList. For not too many elements (<50) consider to use an ArrayList instead. Operations like contains() looping over 50 elements is usually fast enough.
- Use an array instead of an ArrayList, it has less space overhead.
- Use primitives instead of wrappers, e.g. a long instead of a Long.
- String.substring() always shares the backing char[] with the original String. This holds for both the IBM and the Hotspot JRE. If you create small String(s) out of a big String with substring (a rare case) and you only want to retain a small part of the whole char[] by small one(s), prevent retaining the original big char[] by:
leanString = new String(substring.toCharArray()); // only to be considered if substring is created by String.substringNote that for StringBuilder.substring this is not needed, since it does make a copy of the sub part of the char array. Also note we use a JVM option (-Djava.lang.string.create.unique=true see IBM APAR) to achieve this, to remove waste by stringBuilder.toString(). This removes most String waste seen before on the IBM JVM. Defensively making Strings lean before putting them in a session or memory cache is therefore no longer needed. It actually introduces extra time overhead, therefore it is discouraged now.
- For empty lists or other collections, use references to predefined (singleton) empty collections, like Collections.emptyList().
- Use String.intern() to put frequently occurring strings in the single String pool in the JVM, like country, countryCode.
- Use normalization: e.g. NaturalPerson references BranchImpl and only about 300 BranchImpls exist, these can be put in a Map in application scope.
- Calendar is a large object containing about 540 bytes, this can usually be replaced by a Date, java.time.[Local/Zoned]DateTime or a even most compact: a long.
- For a HashMap with Enum keys, use EnumMap. EnumMaps are represented internally as arrays. This representation is extremely compact and efficient.
Observation: ModelMaps are implicitly added to the session. Spring BindingAwareModelMaps are implicitly and automagically added to the portlet session by Spring MVC under certain conditions. A Controller with the following code is problematic:
@ActionMapping
public void action(ModelMap model) {
...
}
@RenderMapping
public String render(ModelMap model) {
...
} Spring will inject the proper data types. The problem is that with portlets, a browser redirect always happens in between the action and the render. A HTTP POST-Redirect-GET happens in the browser and where can Spring get the model from to inject in the render method? Right, from the session!
Problem: ModelMaps are rather large objects containing explicitly added data and administrative data from Spring. They are added to the Portlet session implicitly. They stay in the session for some time: during session activity and 30 minutes (HTTP timeout) after it, in case the user does not exit explicitly. They occupy heap space during that time, for every user.
Solution: Make sure that there is no need for Spring to put ModelMaps or @ModelAttributes in the session. Remove the ModelMap from the render method argument list and create a new local ModelMap to use in the render request scope. If you really need to pass attributes from the action to the render, put them explicitly in the session in the action and make sure to remove them from the session as soon as possible. Do not remove the attributes from the session needed for the rendering process in the render method itself, as that would make page refreshes (or other re-renderings) impossible.
Render parameters might be an option, see TMSU01.
Observation: Form validation in an action request with ModelMaps or ModelAttributes is convenient, but they stay in the session after validation.
Problem: ModelMaps are rather large objects and still take heap space while they are not needed anymore.
Solution: use the ModelAttribute in the validation error flow and clear the ModelMap right after validation in the normal/happy flow, which then also happens when the user has resolved the input validation errors and continues in the happy flow. This is the approach the OI project has taken. See the following example code:
@ActionMapping
public void action(@ModelAttribute SomeForm form, BindingResult errors, ActionResponse response, ModelMap map) {
// Validate the form, on error fill the BindingResult errors object and return the name of the render
// (In this case use the implicit model by Spring to render the original screen including the error)
validator.validate(form, errors);
if (errors.hasErrors()) {
...
response.setRenderParameter("error","true");
return;
}
...
// By clearing the map it won't be persistent in session by Spring
map.clear();
}
@RenderMapping(params="error=true")
public String renderErrorPage(…) {
// Make use of the implicit model of Spring. This model contains the original model as well as the error binding (for each field)
return "page.ftl";
}Observation: A list of static values is reloaded for each user session. Lists Of Values (LOV's) which are more of less static, like countries, currencies, etc., are fetched from a back-end service which is cached in the database. They are stored in each request scope or worse, in each portlet session.
Problem: Since these values are identical for each user and constant for a relatively long period of time, there is no need to store them separately for each user on the heap and call the service/database cache for each request. This introduces extra heap space usage and longer response times.
Solution: Store LOV's only once in memory (best), or only once per application (good enough); in a memory cache or in application/portlet scope.
This section now has grown out on its own as JavaDataAccessPerformance.
See our presentation on Java Performance Pitfall: improper use of XML/JSON and remoting.
XML/SOAP/MQ is used for remoting as well as XML/SOAP/HTTP.
Observation: MQ is used for synchronous calls.
Problem: MQ is relatively slow because of its asynchronous nature and configurations like persisted messaging.
Solution: Use HTTP for synchronous calls.
Observation: XML-Object Binding is too slow.
Problem: XML and SOAP are verbose formats and expensive to process, especially parse.
Solution: If XML or SOAP handling proves to be a serious bottleneck in performance tests, consider to migrate to an alternative like the better performing RMI or a binary web service protocol, like Hessian or Google protocol buffers. Both are supported by the Spring framework. If XML is mandatory, consider VTD-XML, VTD-XML on Wikipedia.
Observation: XMLBeans is used.
Problem: XMLBeans introduces high memory usage and inferior performance for large documents.
Solution: Migrate XMLBeans to JAXB
Observation: JAXBContext is created for each user transaction.
Problem: JAXBContext creation is expensive because it does classloading and other expensive things.
Solution: Since JAXBContext objects are thread safe, they can be shared between requests and reused. So, reuse created instances, e.g. as one singleton per application.
private static final JAXBContext JAXB_CONTEXT;
static {
try {
JAXB_CONTEXT = JAXBContext.newInstance(X.class.getPackage().getName());
} catch (JAXBException e) {
throw new YException(e);
}
}Rule name: implemented, will give false positives: any construction in a method will be flagged. Construction in a static block, the safe way, will not be flagged.
Observation: XMLGregorianCalendar and GregorianCalendar are used with JAXB for dates.
Problem: XMLGregorianCalendar and GregorianCalendar are large objects on the heap, involving substantial processing. To create a new XMLGregorianCalendar, the poorly performing DatatypeFactory is used. A DatatypeFactory.newInstance() is executed, which goes through the complete service look up mechanism, involving class loading.
Solution: Add a converter for alternative date handling with java.time.LocalDateTime instead of default XMLGregorianCalendar.
Example:
import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
public class DateConverter {
private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ISO_LOCAL_DATE;
public static LocalDate parseDate(final String date) {
return LocalDate.from(DATE_FORMATTER.parse(date));
}
public static String formatDate(final LocalDate date) {
return DATE_FORMATTER.format(date);
}
}corresponding JAXB binding configuration:
<jxb:globalBindings>
<jxb:javaType name="java.time.LocalDate" xmlType="xs:date"
parseMethod="com.company.package.DateConverter.parseDate"
printMethod="com.company.package.DateConverter.formatDate" />
</jxb:globalBindings>Note that LocalDateTime is faster than DateTime, however, be aware of its time zone (none) and daylight saving (DST) properties.
Observation: many JAXBContextImpl instances are used.
Problem: JAXBContextImpl objects are large objects on the heap, wasting memory
Solution: Handle as many XSDs per JAXBContextImpl as possible. Ideally just one per application. See: stackoverflow
Observation: JAXB Marshaller, Unmarshaller or Validator is shared by threads.
Problem: JAXB Marshaller, Unmarshaller and Validator are not thread-safe.
Solution: Create a new instance every time you need to marshall, unmarshall or validate a document. If it turns out to be a bottleneck in profiling, consider to pool the instances. More details here.
Observation: XML messages between internal systems are validated in production.
Problem: XML validation is expensive, comparable to parsing. (javax.xml.validation.Validator.validate or org.springframework.xml.validation.XmlValidator)
Solution: Validate messages produced during testing, and once the message format and semantics are valid, switch off validation. Note that load tests should be run production like, so with validation switched off like in production.
Observation: XML related XXXFactory.newInstance() is called repeatedly.
Problem: Upon instance creation of javax.xml.transform.TransformerFactory, javax.xml.parsers.DocumentBuilderFactory, javax.xml.soap.MessageFactory or javax.xml.validation.SchemaFactory,
i.a. the file system is searched for an implementing class in a jar file. This is expensive. The factories are not thread-safe, so they cannot simply be made static and/or shared among threads.
Loading the classes every time means excessive class loading, it results in poor performance.
Solution:
- Preferably create the factory only once. Use a lock to guard the factory, e.g. with a synchronizing wrapper. Be aware of possible contention, so with high load a better solution is:
- To prevent contention, use
ThreadLocals, see example below. - Or a pooling factory solution with for instance a
BlockingQueueof a few factories. - Simple solution: use jvm / system properties to specify your default implementation class. This at least prevents the biggest bottleneck: class path scanning.
Examples:
- Using JVM parameters
-Djavax.xml.parsers.DocumentBuilderFactory=org.apache.xerces.jaxp.DocumentBuilderFactoryImpl
-Djavax.xml.transform.TransformerFactory=org.apache.xalan.processor.TransformerFactoryImpl //old
-Djavax.xml.transform.TransformerFactory=net.sf.saxon.TransformerFactoryImpl //newer
-Djavax.xml.transform.TransformerFactory=com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl //alternative
-Djavax.xml.soap.MessageFactory=org.apache.axis.soap.MessageFactoryImpl
-Djavax.xml.validation.SchemaFactory=com.saxonica.ee.jaxp.SchemaFactoryImpl
- In code:
static {
System.setProperty("javax.xml.transform.TransformerFactory", "com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl");
System.setProperty("javax.xml.soap.MessageFactory", "com.sun.xml.internal.messaging.saaj.soap.ver1_1.SOAPMessageFactory1_1Impl");
}Note: The implementation class of the factory actually used might be just the default class specified in the factory interface code. The service loading mechanism will use this class when no other implementor is found in your classpath.
Find out from a heap dump or classloading logging (-verbose:class) which one is actually used in your app.
Here it shows in a heap dump in VisualVM as only class with objects retaining bytes:
Note: More on TransformerFactory and caching compiled templates, see IBM's XSLT transformations cause high CPU and slow performance.
Example code ThreadLocal for TransformerFactory:
private static final ThreadLocal<TransformerFactory> TRANSFORMER = ThreadLocal.withInitial(TransformerFactory::newInstance);Observation: javax.xml.bind.JAXB utility class is used for marshalling or unmarshalling
Problem: the JAXB class is not optimised for performance and multi-threaded usage. For instance, if multiple types are marshalled, with multiple JAXBContext's, the JAXBContext's are not reused. When URLs are supplied, the context is fetched outside of http connection pools with proper timeout properties and connection sharing. No validation is performed (if you need that, better skip validation when you can for better performance).
Solution: use JAXB API directly for marshalling and unmarshalling to gain all the performance benefits as described in IUOXAR04 and IUOXAR06.
Instead of using this:
JAXB.unmarshal(response, X.class)create a reusable JAXB_CONTEXT via JAXBContext.newInstance(new Class[]{X.class}) (as described in IUOXAR04)
and use as follows:
Unmarshaller u = JAXB_CONTEXT.createUnmarshaller();
u.unmarshal(...);Observation: Jackson's ObjectMapper or JsonMapper is created in each method call.
Problem: ObjectMapper/JsonMapper creation is expensive in time because it does much class loading.
Solution: Since Jackson's ObjectMapper/JsonMapper objects are thread-safe after configuration in one thread,
they can be shared afterwards between requests and reused. So, an option is to reuse created and configured instances, from a static field.
Better is using and sharing ObjectReaders and ObjectWriters created from ObjectMapper since they are immutable and therefore guaranteed to be thread-safe.
Rule name: ObjectMapperCreatedForEachMethodCall
Observation: An ObjectMapper is used as field; an existing ObjectMapper is modified.
Problem: Configuring/modifying an ObjectMapper is thread-unsafe.
Helpful: Only configure objectMappers when initializing: right after construction, in one thread.
Solution: Create configured ObjectReaders and ObjectWriters from ObjectMapper and share those as field, since they are immutable and therefore guaranteed to be thread-safe.
Exceptions: A convertValue method is not provided by Reader/Writer, therefore use of an ObjectMapper as field cannot easily be avoided in this case. The AvoidObjectMapperAsField rule is not applied.
Also when used like jaxMsgConverter.setObjectMapper(objectMapper) it is not considered a violation.
Rule names: AvoidObjectMapperAsField, AvoidModifyingObjectMapper
Example:
class ToAvoidStyle {
private static final ObjectMapper staticObjectMapper = new ObjectMapper(); // bad by AvoidObjectMapperAsField
private final ObjectMapper mapperField = new ObjectMapper(); // bad by AvoidObjectMapperAsField
static {
staticObjectMapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
}
public ToAvoidStyle() {
mapperField.setSerializationInclusion(JsonInclude.Include.NON_NULL);
}
ObjectMapper bad(ObjectMapper mapper) {
mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL); // bad by AvoidModifyingObjectMapper
return mapper;
}
}
class ExceptionCase {
private static final ObjectMapper staticObjectMapper = new ObjectMapper(); //accepted by AvoidObjectMapperAsField
private final ObjectMapper mapperField = new ObjectMapper(); //bad by AvoidObjectMapperAsField
public UserProfileDto getUserProfileDto(UserProfile userProfile) {
return staticObjectMapper.convertValue(userProfile, UserProfileDto.class);
}
}
class GoodStyle {
private static final ObjectWriter staticObjectWriter =
new ObjectMapper().setSerializationInclusion(JsonInclude.Include.NON_NULL).writer(); // good
}Observation: A Gson object is created in each method call.
Problem: Gson creation is expensive.
A JMH benchmark shows a 24x improvement reusing one Gson instance versus a new instance each call.
Solution: Since Gson objects are supposed to be thread-safe,
they can be shared. So reuse created and configured instances from a static field. Take care to use (custom) thread-safe adapters/serializers.
Note that the default Gson com.google.gson.DefaultDateTypeAdapter adapter uses thread-unsafe SimpleDateFormat and the adapter synchronizes on the List of DateFormats to avoid concurrency issues.
This might be an issue in high load/performance environments. Consider using a non-synchronized thread-safe adapter for dates.
Rule name: GsonCreatedForEachMethodCall
Example:
public class GsonReuse {
private static final Gson GSON = new Gson();
public String toJsonBad(List<String> list) {
return new Gson().toJson(list); // bad
}
public String toJsonGood(List<String> list) {
return GSON.toJson(list); // good
}
}Observation: XPath is used to navigate through elements and attributes in an XML document in a wide scope.
Problem: XPath performance is bad when retrieving within a wide scope of the document, e.g. "//".
Solution: Make sure the XPath expressions use a scope as narrow as possible.
Observation: The XPathExpression is created and compiled every time.
Explicitly compiled:
XPathFactory.newInstance().newXPath().compile(xPathExprString).evaluate(..) or implicitly compiled in evaluate:
xpath.evaluate(xPathExprString, doc, XPathConstants.NODESET);is used. The new factory, new XPath, and the compiled expression are hard to cache, because those objects are not thread-safe.
Problem: Creation and compilation of the same expression is performed on every node retrieval, several times per user request. This takes CPU cycles, unnecessarily.
Solution: There is no easy solution, other than not using XPath. Caching in a ThreadLocal might be an option, however, this introduces some complexity.
See: XPath performance tweaks.
Rule name: AvoidRecompilingXPathExpression
Example:
Using ThreadLocal for XPathExpression:
class Bad {
private static final String xPathQuery = "//book[author='Isaac Asimov']/title/text()";
public static NodeList bad1(Document doc) {
XPath xpath = XPathFactory.newInstance().newXPath();
XPathExpression expr = xpath.compile(xPathQuery); // bad
return (NodeList) expr.evaluate(doc, XPathConstants.NODESET);
}
public static NodeList bad2(Document doc) throws XPathExpressionException {
XPath xpath = XPathFactory.newInstance().newXPath();
return (NodeList) xpath.evaluate(xPathQuery, doc, XPathConstants.NODESET); // bad
}
}
class Good {
private static final ThreadLocal<XPathFactory> tlFac = ThreadLocal.withInitial(XPathFactory::newInstance);
private static final ThreadLocal<XPathExpression> tlExpr;
static {
XPath xpath = tlFac.get().newXPath();
try {
XPathExpression expr = xpath.compile("//book[author='Isaac Asimov']/title/text()");
tlExpr = ThreadLocal.withInitial(() -> expr); // good
} catch (XPathExpressionException e) {
throw new RuntimeException(e);
}
}
public static NodeList good(Document doc) throws XPathExpressionException {
return (NodeList) tlExpr.get().evaluate(doc, XPathConstants.NODESET); // good
}
}Observation: XPath is used, implemented by Xalan. XPath implementation has bad performance.
Problem: XPath is reported to have bad performance, see stack overflow and XalanJ Jira issue.
Solution:
1. Avoid the use of XPath.
2. If avoiding XPath turns out to be very difficult, check if the reference above applies and if so, use JVM option (or similarly, System.setProperty in code):
-Dorg.apache.xml.dtm.DTMManager=org.apache.xml.dtm.ref.DTMManagerDefault
for the IBM J9 JVM. We have measured a speedup factor of 6.0 for IBM JVM 8.
For other JVMs, the following might work:
-Dcom.sun.org.apache.xml.internal.dtm.DTMManager=com.sun.org.apache.xml.internal.dtm.ref.DTMManagerDefault
Depending on e.g. which one shows up in your Java stack traces / heap dump, for example for IBM JVM 8, in VisualVM with filter DTMManager:
3. Use CachedXPathAPI, see Ways to increase the performance of XML processing in Java - Case 2. Be aware of higher memory usage. Note that CachedXPathAPI object is thread-unsafe.
Alternative approach: If XPath evaluation still turns out to be a bottleneck by profiling, consider to switch to VTD-XML
Observation: Spring WS-Addressing is used with Action-annotated resolution
Problem: When using Spring WS-Addressing and the @Action annotation, Spring-WS uses AddressingVersions which use Spring's AbstractAddressingVersion. This uses XPath with Xalan implementation to extract headers from the addressing header and has bad performance. See Spring Jira issue the RASS team submitted. For parsing incoming messages this is a problem, for outgoing messages XPath usage is not a problem.
Solution: Avoid the use of Spring WS-Addressing. In non-avoidable: add JVM option for partial solution, see above.
Alternative approach: Add custom version objects to the AnnotationActionEndpointMapping during startup. These custom versions should resolve the headers without a performance hit.
Alternative approach: If XPath evaluation still turns out to be a bottleneck by profiling, consider to switch to VTD-XML
See our presentation on Java Performance Pitfall: improper logging.
Observation: Concatenation is used in unconditionally executed logging statements:
LOG.debug("Length: " + length + ", currency: " + currency);Problem: Concatenation happens before the debug, trace or info method executes, so independent of the need to actually log. Concatenation is relatively expensive.
Solution: Use SLF4J with the {} pattern and pass the variables without concatenating them
LOG.debug("Length: {}, currency: {}", length, currency);Rule name: UnconditionalConcatInLogArgument.
Observation: A logging String is built unconditionally:
while(..) {
...
logStatement.append("Found page parameter with key '" + key + "' and value '" + value + "'\n"); // bad
}
LOG.debug("Note: {}", logStatement);Problem: String building, concatenation and/or other operations happen before the debug, trace or info method executes, so independent of the need to actually log. Concatenation is relatively expensive.
Solution: Built the String conditionally on the log level, within an if statement. A nicer solution is possible with SLF4J2, by utilizing a lambda expression:
LOG.debug("Found page key-value pairs: {}", () -> buildPageKeyValueLogString()); // good
The buildPageKeyValueLogString method is deferred and only executed if needed, by SLF4J.
Rule name:: AvoidUnconditionalBuiltLogStrings
Observation: An operation is executed on a log argument, irrespective of log level. Like:
LOG.debug("ACTUAL DOWNLOAD: EndDate Download Date: {}", String.format("%1$tR", endDateDownloadDate); // bad
LOG.trace("StepExecution: \n{}", stepExecution.toString()); // bad
LOG.debug("Complete Soap response: {}", getSoapMsgAsString(context.getMessage())); // badProblem: toString(), String.format or some other operation or method call is executed irrespective of log level. This may include formatting, concatenation, reflection and other wasteful processing. For example, the above line with toString seems rather harmless, however, you might change your mind if you see the toString implementation of StepExecution (at the bottom of the page.)
Solution: Remove the toString() since this is already invoked conditionally inside SLF4J. If formatting is really needed, execute it conditionally or in its toString method. Or even nicer, use a lambda expression with SLF4J2, see IL02 above.
LOG.debug("ACTUAL DOWNLOAD: EndDate Download Date: {}", endDateDownloadDate); // good
LOG.trace("StepExecution: \n{}", stepExecution); // good
if (LOG.isDebugEnabled()) { // good
LOG.debug("Complete Soap response: {}", getSoapMsgAsString(context.getMessage()));
}Rule name: UnconditionalOperationOnLogArgument.
Observation: MDC values are added for logging, but not removed, or not in all cases.
MDC = Mapped Diagnostic Context, used to provide context information (e.g. userId) for all subsequent log calls in a user transaction.
Problem: MDC values can leak to other user transactions (requests) and log incorrect information. This can happen because MDC is implemented with ThreadLocals and a thread holding the ThreadLocal is reused for another user transaction after it has finished its current transaction. Additionally, memory is wasted by leaked MDC values.
E.g.:
MDC.put("UserId", userId);with no corresponding
MDC.remove("UserId");Solution: Determine the MDC-value lifecycle and remove the MDC value in the proper place. Put the remove in a finally clause so it is also removed in case of exceptions.
See also: Automating access to the MDC.
Rule name: MDCPutWithoutRemove.
Observation: Synchonous logging is used for much data.
Problem: Logging is I/O which can take much time away from the user request/response.
Solution: Use Asynchronous logging. In logback you can use: ch.qos.logback.classic.AsyncAppender. Log lines are put in a queue and the user does not have to wait for the actual logging, which is handled in a separate thread. Think about i.a. maximum queue size, data loss and what information to transfer to the logging thread (MDC).
Observation: A log argument is created unconditionally, irrespective of log level.
Problem: Creation of a log argument with a toString or other operation(s) may be expensive, while depending on the log level, the result may not be used.
Solution: Create the log argument conditionally on the log level, within an if statement. For just 'obj.toString()', just pass 'obj' to the log method and leave it to SLF4J to call toString() only if needed. See IL02 for a nice solution using a lambda.
Rule name: UnconditionalCreatedLogArguments
Observation: Logging configuration uses location information (%L or %line, %M or %method, %C or %class, %F or %file, %l or %location).
Problem: Getting location information of a log statement requires taking a snapshot of the stack and walking the stack trace, which is expensive.
Note: It is especially expensive for asynchronous loggers.
Solution: Only use location information in debug or error logging.
See: logback.layouts#method and log4j2 Layouts
Improper streaming may result in unwanted large object allocations. The IBM JVM has the possibility to log large object allocations to analyse this. Large object allocation logging can be enabled by using the following jvm argument with an appropriate filter value:
-Xdump:stack:events=allocation,filter=#6m
Logging of the allocation stack traces will be in native_stderr.log.
Observation: ByteArrayOutputStream or StringWriter default constructor is used for large strings/streams, or a capacity smaller or equal to the default.
Problem: By default an initial buffer of 32 bytes or 16 characters is created, respectively. If this is not enough, a new byte/char array will be allocated and contents will be copied into the new array. The old array becomes garbage to be collected and copying takes processing time.
If you know what the minimum or typical size will be, this garbage and processing time are wasted.
Solution: Presize the ByteArrayOutputStream or StringWriter with an initial capacity such that a resize is not needed in most cases. By using the ByteArrayOutputStream or
StringWriter alternative constructor.
If the buffer is send/received through I/O, then the size should be 8192 bytes or a multiple of it, 8 KiB being the default TCP buffer size on most systems. This value of 8 KiB (kibibyte) is used as default buffer size in all I/O buffering classes in the Java libraries.
If the buffer contents is not send or receiceved from I/O, than take 4 kiB or a multiple of it, 4 KiB being the default memory page size.
Rule name: AvoidInMemoryStreamingDefaultConstructor
Example:
class Bad {
public static void bad() {
ByteArrayOutputStream baos = new ByteArrayOutputStream(); //bad
StringWriter sw = new StringWriter(); //bad
baos = new ByteArrayOutputStream(32); //bad - not larger than default
}
}
class Good {
public static void good() {
ByteArrayOutputStream baos = new ByteArrayOutputStream(8192); // 8 kiB
StringWriter sw = new StringWriter(2048);
}
}Observation: An unbuffered input or output is used. Applicable to InputStream, OutputStream, Reader and Writer
Problem: Each write() or read() call is a system call which is relatively expensive. Doing a system call for every byte or char is very inefficient.
Solution: By buffering, a bunch of data is given to the system call at once to process, making the work per byte much less and the total much more efficient. Examples:
BufferedReader in = new BufferedReader(new FileReader("IOStreamDemo.java"));
BufferedOutputStream bufferedOutput = new BufferedOutputStream(new FileOutputStream("yourFile.txt"));This especially applies to direct file streaming. Access through ClassLoader.getResourceAsStream / URL.openStream where URL is of FILE or HTTP protocol, is buffered by the implementing class, e.g. sun.net.www.protocol.file.FileURLConnection.
Observation: All bytes of a large file are loaded into memory. E.g. for uploading or downloading documents, for instance to determine the mime type or create a digest.
Problem: Large objects are allocated on the heap, up to e.g. 50 MB. We also observed 300 MB and even 1 GB in back-end systems. This likely triggers long gc pauses for compaction or may trigger out of memory crashes.
In the next example, there are two large byte arrays in memory: one in baos and the other is the returned byte array since a copy is made in toByteArray().
private byte[] zipData() {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
ZipOutputStream zos = new ZipOutputStream(baos);
// stream 1 GB data from input to output stream (zos) here
return baos.toByteArray();
}Example 2:
class Bad1 {
void bad(Path path) {
byte[] fileBytes = Files.readAllBytes(path); // bad
List<String> fileLines = Files.readAllLines(path); // bad
// now process bytes / lines
}
}Solution: Stream-through: use streaming all the way, don't store the whole thing in memory, don't use byte arrays. A mime type is determined from the first few bytes of the file, don't read in all 50 MB - 1 GB for that. Often, functionality can be achieved in a streaming way, i.e. a digest can be computed in a streaming way.
Example 1 improved:
private void zipData() {
FileOutputStream dest = new FileOutputStream(file);
ZipOutputStream zos = new ZipOutputStream(new BufferedOutputStream(dest));
// stream 1 GB data from input to output stream (zos) here, which will be written to file
}Example 2 improved, with some example processing added:
void good(Path in, Path out) throws IOException {
try (BufferedReader reader = Files.newBufferedReader(in);
BufferedWriter writer = Files.newBufferedWriter(out, APPEND)) {
String line = reader.readLine();
if (line.contains("ERROR")) {
writer.write(line);
writer.newLine();
}
}
}Note: For small files say < 10 kbyte and not many being processed at the same time, this probably is not an actual problem.
Observation: Streaming SOAP with MTOM is used for uploading or downloading documents, with embedded base64 encoded PayloadData.
Problem: The heap shows much heap usage by XMLStreamReaders, the IBM StAX implementation caches these. For large documents and high load, heap usage can become very high, such that it will cause an out-of-memory error crash.
Solution: IBM's description PM42465: Excessive memory usage by XMLStreamReader pool may lead to OutOfMemoryError offers a work around. In project experience (5-10 MB documents) this did not help enough, we still got an occasional OOM. Real solution was
- switch to binary streaming upload with apache HTTP client. This is also used for back-end service calls with large data transfers, not just uploads.
- use binary attachments. In this solution, the PayloadData SOAP element only contains a reference to a separate MIME block in the POST body, containing a reference to a file with the binary data. See Attachments over webservices. This works well in project.
Validate with large object allocation logging that no large objects are allocated and buffered streaming is working correctly, by using a (J9) jvm argument like:
-Xdump:stack:events=allocation,filter=#6m
Logging of allocation stack traces will be in native_stderr.log.
Observation: FileInputStream and FileOutputStream are used without buffer.
Problem: With FileInputStream and FileOutputStream, file access is not buffered.
The stream is read-from/written-to file byte by byte, where each operating system call has its overhead, which makes it slow.
Solution: Use buffering to read/write a chunk of bytes at once with much lower overhead. Use BufferedOutput/InputStream which has a buffer size of 8 kB by default to write at once.
Make sure to close (flush) a BufferedOutputStream after the last write, otherwise the last part may not be written to file.
Rule name: BufferFileStreaming.
Example:
class BufferFileStreaming {
void bad1(String inputFilename, String outputFilename) {
try (FileInputStream fis = new FileInputStream(inputFilename)) { // bad
//use fis
}
try (FileOutputStream fos = new FileOutputStream(outputFilename)) { // bad
//use fos
}
}
void good1(String inputFilename, String outputFilename) {
try (InputStream bfis = new BufferedInputStream(new FileInputStream(inputFilename))) { // good
//use bfis
}
try (OutputStream bfos = new BufferedOutputStream(new FileOutputStream(outputFilename))) { // good
//use bfos
}
}
}Observation: No buffering is added to the use of Files.newInputStream and Files.newOutputStream.
Problem: Files.newInputStream or Files.newOutputStream is not buffered. The stream is read/written to file byte by byte, where each operating system call has its overhead which makes it slow.
Solution: Use buffering to read/write a chunk of bytes at once with much lower overhead. Use e.g. BufferedInputStream or BufferedOutputStream which has a buffer size of 8 kB by default to write at once.
Make sure to close (flush) a BufferedOutputStream after the last write, otherwise the last part may not be written to file.
Rule name: BufferFilesNewStream.
Example:
class Foo {
OutputStream badOut(String path) throws IOException {
return java.nio.file.Files.newOutputStream(Paths.get(path)); // bad
}
OutputStream goodOut(String path) throws IOException {
return new BufferedOutputStream(java.nio.file.Files.newOutputStream(Paths.get(path)));
}
InputStream badIn(String path) throws IOException {
return java.nio.file.Files.newInputStream(Paths.get(path)); // bad
}
OutputStream goodIn(String path) throws IOException {
return new BufferedInputStream(java.nio.file.Files.newInputStream(Paths.get(path)));
}
}Observation: A security provider is re-created, that is, created in a method called more than once.
Problem: Creating a security provider is expensive because of loading of algorithms and other classes.
Additionally, it uses synchronized which leads to lock contention when used with multiple threads.
Solution: This only needs to happen once in the JVM lifetime, because once loaded, the provider is available from the Security class.
Create the security provider only once: only in case it is not available from the Security class, yet.
Rule name: AvoidRecreatingSecurityProviders.
Example:
class Foo {
public Cipher initBlowfishBad() throws GeneralSecurityException {
Security.addProvider(new BouncyCastleProvider()); // bad
// build a Cipher
}
public Cipher initBlowfishGood() throws GeneralSecurityException {
Provider bouncyCastleProvider = Security.getProvider(BouncyCastleProvider.PROVIDER_NAME);
if (bouncyCastleProvider == null) {
bouncyCastleProvider = new BouncyCastleProvider();
Security.addProvider(bouncyCastleProvider);
}
// build a Cipher
}
}Note: An addProvider call inside a static main method or inside a @PostConstruct annotated method is not reported as a violation since it is assumed to be called only once.
Observation: A Spring ApplicationContext is re-created (to complete with example)
Problem: When a XXXApplicationContext is created, all Spring beans are initialized, wired and component scanning may take place. Component scanning involves extensive class path scanning which is expensive.
Solution: Create the ApplicationContext only once in the application deployed/live time.
Observation: A Spring component scan is used explicitly (to complete with example)
Problem: A component scan uses extensive class path scanning which is expensive.
Solution: Specify explicitly**.** And/or use Java 9+ modules which have an index.
Observation: Reflection is used unnecessarily, for instance like:
public boolean equals(final Object arg0) {
return EqualsBuilder.reflectionEquals(this, arg0);
}Problem: Reflection is relatively expensive.
Solution: Avoid to use reflection. Use the non-reflective, explicit way, see equals and hashCode section below.
Rule name: prototype ready, hits on EqualsBuilder.reflectionEquals and HashCodeBuilder.reflectionHashCode of Apache commons lang.
When multiple threads access the same object, access it in a thread safe way. Getting thread safety right and not hindering performance is difficult. Locking with the synchronized keyword may introduce lock contention under load, which is bad for performance. To make threading aspects easier to understand in source code, we recommend the use of Java Concurrency In Practice annotations.
Observation: The synchronized method contains much code and/or takes time. For instance by doing I/O.
Problem: Other threads that want to access the synchronized method on the same object have to wait until the 'owner' exits the method.
Solution: Minimize the synchronized code block, don't execute an alien call in a synchronized block, don't do I/O there. See Concurrency In Practice.
Observation: The broken double checked locking idiom is used. Example:
// "Double-Checked Locking" idiom - Broken / Wrong!
class Foo {
private Helper helper = null;
public Helper getHelper() {
if (helper == null) {
synchronized(this) {
if (helper == null) {
helper = new Helper();
}
}
}
return helper;
}
}Problem: Other threads may not see the initialized helper set by one thread. They could also see a partly initialized helper, leading to corruption. Sometimes. See double checked locking idiom.
Solution: Use a volatile reference to make sure the assignment to it is published to all threads in a thread-safe way. For example for a lazy initialized, immutable map:
class Foo {
private volatile Map map = null;
public Map getMap() {
if (map == null) { // 1
synchronized(this) { // 2
if (map == null) { // 3
map = Collections.unmodifiableMap(buildMap()); // 4, 5
}
}
}
return map;
}
// 1. volatile, so every thread sees current value of reference
// 2. make sure check-and-modify of map is 'atomic'
// 3. make sure map is not initialized more than one time
// 4. only read-access of map, so thread-safe
// 5. make sure keys and values are thread-safe, too
}This solution is usually good enough. A more optimized version using a local variable, yet more complicated can be found on wikipedia. See also: double-checked-locking--clever--but-broken-by-Brian-Goetz.
Observation: A data structure like a Collection or Map or object like applicationContext is constructed once by one thread and consecutively read-only by one or more other threads. No thread-safety is used.
Problem: It is not guaranteed that the other threads will ever see the constructed data structure, and they may see a partially constructed version, leading to corruption of data. Sometimes.
Solution: There are three options:
- use a static final variable and assign the fully constructed immutable datastructure to it during class loading;
- use a volatile reference and assign the fully constructed immutable datastructure to it once during initialization;
- use the double checked volatile locking idiom, see solution of the previous item.
For all three solutions holds that the data structure itself does not need to be thread-safe, only that is must be fully constructed and not changed after assignment to its reference. So, it better be immutable to guarantee this. See Java Concurrency In Practice, Chapter 3.5.
Observation: Java Concurrency In Practice annotations are not used. (net.jcip version with full documentation) (new package location: javax.annotation.concurrent)
Problem: Often, the intended thread-safety policies of a class are unclear, leading to hard-to-find concurrency bugs. These annotations describe this intent.
Solution: Utilize the annotations: @GuardedBy, @Immutable, @ThreadSafe and possibly @NotThreadSafe, as well. These will communicate the intended thread-safety promises to both users and maintainers. In addition, IDE's and FindBugs/Sonar use these annotations to check thread-safety. For example:
@Immutable
public class String {}
@NotThreadSafe
public class StringBuilder{}
@ThreadSafe
class Example {
private final Object lock = new Object();
@GuardedBy("lock")
private Stuff internal = new Stuff();
public void work() {
synchronized(lock) {
workWith(internal.fiddle());
}
}
}Observation: A class has setters with the only purpose to initialize it.
Problem: Setters make the class mutable while it may otherwise be immutable. Immutability eases multithreading.
Solution: Use the builder pattern instead of setter methods and make sure it is immutable.
Observation: Mutable objects are used in HttpSession or PortletSession.
Problem: Objects in the HttpSession are accessed by multiple threads, for example consecutive threads from the thread pool, or concurrent portlet threads. HttpSession.getAttribute, setAttribute and removeAttribute are synchronized. A call to setAttribute is needed after modifications: it will make the reference to the object visible to other threads, as well as all modifications to the object itself which happened before that. However, there is no atomicity of the modification operations on the object itself, it may be interrupted by other threads and become in inconsistent state.
Solution: Objects used in HttpSessions should be immutable. If modification is needed, a new version of the object should be put in session with setAttribute. If this is not possible (why?), be sure to access the object in session in a thread-safe way, using a lock around access and modification. Warning: This is difficult to get right and error prone. This problem and solution is described in full detail by Brian Goetz.
Observation: A singleton, or more general: an object shared among threads, is used with non-final and/or mutable fields and the fields are not guarded (e.g. accessor methods are not synchronized), while typically fields are not intended to change after initialization. This includes Spring @Component, @Controller, @RestController, @Service and @Repository annotated classes for application and session scope and JavaEE bean-managed @Singleton annotated classes.
Problem: Multiple threads typically access fields of a singleton or may access fields in session scoped objects. If a field or its reference is mutable, access is thread-unsafe and may cause corruption or visibility problems. To make this thread-safe, that is, guard the field e.g. with synchronized methods, may cause contention.
Solution: Make the fields final and unmodifiable. If they really need to be mutable, make access thread-safe. Thread-safety can be achieved e.g. by proper synchronization and use the @GuardedBy annotation or use of volatile.
Notes
- Instances of Date, StringBuilder, URL and File are examples of mutable objects and should be avoided (or else guarded) as fields of shared objects. In case mutable fields are final and not modified after initialization (read-only) they are thread safe, however any modification to it is thread-unsafe. Since field modification is easily coded, avoid this situation.
- Instances of classes like ArrayList, HashMap and HashSet are also mutable and should be properly wrapped with e.g. Collections.unmodifiableList after initialization (see TUTC03), made immmutable by e.g. List.copyOf() or accessed thread-safely with e.g. Collections.synchronizedList or thread-safe implementations like ConcurrentHashMap.
- For autowiring/injection, the assignment of the reference is thread safe, so final is not required. The unmodifiable/thread-safe requirement for that field still holds. Also make sure no other thread-unsafe assignment is made to that field.
- In case you are sure the Component is used in single threaded context only (e.g. a Tasklet), annotate the class with @NotThreadSafe to make this explicit.
- Use package private and @VisibleForTesting for methods used for JUnit only.
- Documentation on a.i. using final for threads-safety and from the JLS: An object is considered to be completely initialized when its constructor finishes. A thread that can only see a reference to an object after that object has been completely initialized is guaranteed to see the correctly initialized values for that object's
finalfields. - Also See Java Concurrency In Practice, Chapter 3.5.
Example with issues
@Component
@Scope(WebApplicationContext.SCOPE_SESSION) // similar for e.g. default SCOPE_APPLICATION
public class ReportController extends AbstractController {
private Report data;
private boolean contacted;
private RestTemplate restTemplateOk;
private String name;
public ReportController() {
name = "LaundryReport";
} // unsafe
public void createData() {
data = something();
} // unsafe
public Report getData() {
return data;
} // unsafe
public boolean getContacted() {
return contacted;
} // unsafe
public void setContacted(boolean contacted) {
this.contacted = contacted;
} // unsafe
@Autowired
public void setRestTemplate(final RestTemplate restTemplate) { // autowiring is safe
this.restTemplateOk = restTemplate;
}
}Example with issues solved
@Component
@Scope(WebApplicationContext.SCOPE_SESSION) // similar for e.g. default SCOPE_APPLICATION
public class ReportController extends AbstractController {
@GuardedBy("this") // needed to remove pmd/Sonar violation, enables extra checks
private Report data;
private volatile boolean contacted; // assignment-safe
private RestTemplate restTemplateOk;
private final String name; // assignment-safe
public ReportController() {
name = "LaundryReport";
} // safe because of final
public synchronized void createData() {
data = something();
} // safe because synchonized (@GuardedBy)
public synchronized Report getData() {
return data;
} // safe like previous if data is immutable, or if a copy is retured
public boolean getContacted() {
return contacted;
} // safe because volatile
public void setContacted(boolean contacted) {
this.contacted = contacted;
} // safe because volatile
@Autowired
public void setRestTemplate(final RestTemplate restTemplate) { // autowiring is safe
this.restTemplateOk = restTemplate;
}
}Rule names: AvoidUnguardedMutableFieldsInSharedObjects, AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects, AvoidUnguardedMutableInheritedFieldsInSharedObjects, AvoidUnguardedAssignmentToNonFinalFieldsInObjectsUsingSynchronized, AvoidUnguardedMutableFieldsInObjectsUsingSynchronized
Observation: A static field is mutable or non-final.
Problem: Multiple threads typically access static fields. Unguarded assignment to a mutable or non-final static field is thread-unsafe and may cause corruption or visibility problems. To make this thread-safe, that is, guard the field e.g. with synchronized methods, may cause contention.
The next examples show non-final fields:
private static String fileName = "<none>"; // likely a concurrency bug
private static Logger log = new Logger(); // potential concurrency bugHere the first likely is a concurrency bug (agree?) and the second probably not. The reference is mutable. Usually 'final' can just be added to prevent concurrency problems.
The following example is less obvious, an array is by many considered immutable, which is a wrong assumption: elements can be replaced. Therefore, it has the same risk as the above examples.
private static final String[] QUALIFIERS_Violate = {"alpha", "beta", "milestone"}; // mutableSolution: Make the fields final and unmodifiable. How to make a static final List or Set unmodifiable, see example of PML01. The immutable version of the literal String array:
private static final List QUALIFIERS_Ok = Collections.unmodifiableList(Arrays.asList("alpha", "beta", "milestone"));With Java 9 this can be much more compact:
private static final List QUALIFIERS_Ok = List.of("alpha", "beta", "milestone");or by using Guava immutable collections like ImmutableList:
private static final List QUALIFIERS_Ok = ImmutableList.of("alpha", "beta", "milestone");Note that for primitives Guava has: ImmutableIntArray, ImmutableLongArray and ImmutableDoubleArray.
If they really need to be mutable, make access thread-safe. Thread-safety can be achieved e.g. by proper synchronization and use the @GuardedBy annotation or use of volatile. Consider lock contention.
Rule name: AvoidMutableStaticFields
Observation: A compound statement like i++ or i-=1 is used for a volatile field
Problem: A compound statement like i++, i--, i += 1 or i -= 1 may seem one statement and thread-safe for a volatile field.
However, the operation actually comprises two separate statements executed non-atomically and therefore not thread-safe.
Solution: In stead of volatile, guard the field properly with synchronized, atomics like AtomicInteger or java.util.concurrent locks.
Rule name: AvoidIncrementOrDecrementForVolatileField
Observation: A field is defined as volatile while the class has prototype scope.
Problem: Volatile has some overhead, especially for writes. When getting the bean from the Spring applicationContext,
prototype scope means that each invocation creates a new object so the field is not shared.
Solution: Since only one thread can access the field, there is no need for volatile and it can be removed.
Rule name: AvoidVolatileInPrototypeScope
Observation: A singleton, or more general: an object shared among threads, has a field that is not private.
Problem: A shared object needs to be thread safe. Thread safety of fields cannot be guaranteed if not private:
the fields can possibly be modified from other classes.
Solution: Make the fields private and make sure that they are used in a threadsafe way.
Note: Note: fields are excluded when annotated with: @Autowired/@Value/@Inject.
Rule name: AvoidNonPrivateFieldsInSharedObjects
Observation: Only local variables are accessed in a synchronized block.
Problem: Synchronization has overhead and may introduce lock contention.
Solution: Remove the synchronized statement because local variables are only accessible by the owning thread and are not shared.
Rule name: SynchronizingForLocalVars
Example:
public class Foo {
private Map<String, String> mapField;
protected Map<String, String> bad() {
Map<String, String> addHeaders = MDC.getCopyOfContextMap();
synchronized (this) { // bad, no use
if (addHeaders == null) {
addHeaders = new HashMap<>();
}
}
return addHeaders;
}
}Observation: A JavaEE @Singleton is used with default @ConcurrencyManagement(CONTAINER).
Problem: @Singleton has default @ConcurrencyManagement(CONTAINER) and write locks on the methods. Using defaults is unclear and write locks typically cause much more contention than read locks.
Solution: Make @ConcurrencyManagement and @Lock-s explicit, @Lock on class level or on all public methods. Or use BEAN managed for full control using e.g. synchronized or volatile.
Rule name: DefineConcurrencyForJavaEESingleton
See: Managing Concurrent Access in a Singleton Session Bean
Example:
@Singleton // bad - 1. @ConcurrencyManagement missing 2. @Lock missing on public method
class SingletonBad {
private String state;
public String getState() {
return state;
}
@Lock(LockType.WRITE)
public void setState(String newState) {
state = newState;
}
}
@Singleton // good Container Managed version
@ConcurrencyManagement(ConcurrencyManagementType.CONTAINER)
class SingletonCMGood {
private String state;
@Lock(LockType.READ)
public String getState() {
return state;
}
@Lock(LockType.WRITE)
public void setState(String newState) {
state = newState;
}
}
@Singleton // good Bean Managed version
@ConcurrencyManagement(ConcurrencyManagementType.BEAN)
class SingletonBMGood {
private volatile String state; // thread-safe for the reference, and String is immutable
public String getState() {
return state;
}
public void setState(String newState) {
state = newState;
}
}Observation: A Calendar is unnecessarily created for a Date or time
Problem: A Calendar is a heavyweight object and expensive to create. For example, to copy a Date:
Calendar dateCalendar = Calendar.getInstance();
dateCalendar.setTime(date);
return dateCalendar.getTime();or
long time = Calendar.getInstance().getTimeInMillis();Solution: Avoid use of Calendar, in this example by:
new Date(otherDate.getTime());or
long time = System.currentTimeMillis();Or better yet instead of Date, use a java.time.LocalDateTime, which also has the advantage over java.util.Date that it is immutable.
Rule name: prototype ready, hit on Calendar.getInstance().getTime() usage and on two steps in same block.
Observation: Unnecessary or hidden auto boxing and unboxing, e.g. primitive types from/to corresponding Object types.
Problem: There is a performance penalty, especially when using large arrays and/or compute intensive loops. Can introduce subtle bugs, see example 2.
Solution: Avoid auto boxing and unboxing.
Example 1: subcategoryId is of type Integer:
response.setRenderParameter(RENDER_PARAMETER_SUB_CATEGORY_ID, Integer.toString(subCategoryId));subCategoryId is unboxed to an int to work in the Integer.toString(int) method that expects only a primitive int. Use subcategoryId.toString() to avoid unboxing.
Example 2: categoryId is of type int, valuesAsList is of type List:
if (valuesAsList.contains(categoryId)) {
valuesAsList.remove((Integer) categoryId);
} else {
valuesAsList.add(categoryId);
}categoryId of type int will be boxed to Integer for the contains() and add() and remove() calls on valuesAsList.
Note the use of explicit (Integer) cast to call the correct overloaded remove(Integer) method instead of the remove(int) method. The first will remove the Integer object, the second will remove the Object on index categoryId. Without the explicit cast the latter will be called, resulting in wrong behaviour.
Suggested fix: create a category id variable of type Integer before the if statement so no further implicit boxing is done.
Tip: enable the compiler check on auto boxing/unboxing in eclipse.
Observation: A Calendar field is used in an object with many instances.
Problem: A Calendar field is typically only used for representing a date and time. A java.util.Calender object wastes heap space: it occupies about 540 bytes of heaps space.
Solution: Use a java.time.LocalDateTime, which also has the advantage over java.util.Date that it is immutable. Also even a primitive long type for "milliseconds since epoch" could be used instead, for the most space-efficient solution.
Observation: A LinkedList is used. e.g. to maintain insertion order in a list
Problem: A LinkedList is faster than an ArrayList for insertion and removal of elements of long lists, say with > 100 elements. However, it takes more memory because it maintains a forward and backward pointer for every element. Additionally, for short lists it is never faster.
Solution: Use an ordinary ArrayList in stead. It will maintain insertion order just like the Linked version. Note: a Set and Map don't maintain insertion order, those need the Linked version for that.
Observation: A ConcurrentHashMap is used unnecessarily, that is, for created-only-once and consecutively unmodified data or when used by only one thread.
Problem: A ConcurrentHashMap is particularly greedy in heap usage because of its segments and locks.
Solution: Only use ConcurrentHashMap with multiple threads and when modified. Otherwise, for read-write access by a single thread just use HashMap en with multiple threads and read-only use:
Map readOnlyMap = Collections.unmodifiableMap(initializeHashMap());Observation: A HashMap or HashSet is used with Enum keys.
Problem: A HashMap and HashSet are rather greedy in memory usage.
Solution: Use an EnumMap or EnumSet. It is represented internally with arrays which is extremely compact and efficient.
Map<YourEnumType, String> map = new EnumMap<>(YourEnumType.class);
Set<YourEnumType> set = EnumSet.allOf(YourEnumType.class);Observation: Elements are searched in a List . The list is iterated or streamed until the right (key, value) element is found.
Problem: the time to find element is O(n); n = the length of list.
Solution: a Map is more time efficient, value is found by key. Access time is O(1), provided the hashCode is well defined.
Observation: A Collection is instantiated without an initial capacity.
Problem: If no initial capacity is specified, a default capacity is used, which will rarely be optimal. Failing to specify initial capacities for collections may result in performance issues, if space needs to be reallocated and memory copied when capacity is exceeded.
Solution: Specify an initial capacity, suited for your need.
Observation: A Collection is wrapped with a synchronized wrapper and multiple threads put significant load on it.
Problem: There is only one lock used for accessing the collection by all threads, which may result in lock contention.
Solution: Use a collection which is optimized for access by multiple threads:
- CopyOnWriteXxx collections of java.util.concurrent for mostly read access
- ConcurrentXxx collections of java.util.concurrent for non-mostly read. These utilizes non-blocking/wait-free algorithms or lock striping (see thinking in parallel), for instance ConcurrentHashMap. Or use NonBlockingHashMap of high-scale-lib.
- Add lock striping to your collection with guava's Striped.
Observation: To get an enum value by its defined field, the values are streamed on every call.
Problem: the time to find element is O(n); n = the number of enum values. This identical processing is executed for every call. Considered problematic when n > 3.
Solution: use a static field-to-enum-value Map. Access time is O(1), provided the hashCode is well-defined.
For one String field, usually toString returns that field. Consider to implement a fromString method to provide the reverse conversion by using the map, see the following examples:
Examples:
// BAD
public enum Fruit {
APPLE("apple"),
ORANGE("orange"),
BANANA("banana"),
KIWI("kiwi");
private final String name;
Fruit(String name) { this.name = name; }
@Override
public String toString() { return name; }
public static Optional<Fruit> fromString(String name) {
return Stream.of(values()).filter(v -> v.toString().equals(name)).findAny(); // bad: iterates for every call, O(n) access time
}
}Usage: Fruit f = Fruit.fromString("banana");
// GOOD
public enum Fruit {
APPLE("apple"),
ORANGE("orange"),
BANANA("banana"),
KIWI("kiwi");
private static final Map<String, Fruit> nameToValue =
Stream.of(values()).collect(toMap(Object::toString, v -> v));
private final String name;
Fruit(String name) { this.name = name; }
@Override
public String toString() { return name; }
public static Optional<Fruit> fromString(String name) {
return Optional.ofNullable(nameToValue.get(name)); // good, get from Map, O(1) access time
}
}As a side note, if you are happy with the Fruit name as capitals, you don't need any of the fields and methods and the default implementation of toString and valueOf will do, so Fruit simply becomes:
public enum Fruit { // great, very simple
APPLE,
ORANGE,
BANANA,
KIWI
}With usage: Fruit f = Fruit.valueOf("BANANA");
See:
- This is a special case of #IUOC04.
- Effective Java 3rd Ed, p164 (Use enums).
Observation: Map keys do not implement Comparable.
Problem: If multiple entries end up in the same HashMap bucket, they are stored as LinkedList, and with more than 7 as a red black tree.
The list access time is O(n) and tree access time is only O(log n) which is much faster especially for large n. This tree implementation utilizes the compareTo from the Comparable interface.
If this is not implemented, access will be slow.
Solution: Implement Comparable for your Map keys. Do not use classes for those which don't implement Comparable, like Thread, Class and Object. At least not for Maps which can grow large.
Note: that equals and hashCode must be implemented properly for the keys, and compareTo must be compatible with equals.
Rule name: NonComparableMapKeys
Examples:
class NonComparableMapKeys {
Map<Object, String> oMap; // bad, Object does not implement Comparable
Map<Thread, String> tMap; // bad, Thread does not implement Comparable
Map oldStyleMap = new HashMap(); // cannot check here
void putInOldStyleBad() {
oldStyleMap.put(new Thread(), "value"); // bad
}
Map<Comparable, String> cMap; // good
void putInOldStyleGood() {
oldStyleMap.put("key", "value");
}
}See: Java 8 HashMap keys and Comparable
Note: This rule replaces Sonar rule java:S6411 The key type should implement Comparable, which has shortcomings.
Observation: Set elements do not implement Comparable and by-element access is used.
Problem: A Set is implemented with a Map. If multiple entries end up in the same HashMap bucket, they are stored as LinkedList, and with more than 7 as a red-black tree.
The list access time is O(n) and tree access time is only O(log n) which is much faster for large n. This tree implementation utilizes the compareTo from the Comparable interface.
If this is not implemented, access by element will be slow. Iterating through the elements does not suffer from this slow access because no lookup by key in the map is involved.
Access methods by element which are affected: contains[All], retainAll, remove[All].
Solution: Implement Comparable for your own Set elements. Avoid using large Sets with elements of types which don't implement Comparable, like Thread, Class and Object. At least when using access by element.
Note that equals and hashCode must be implemented properly for the elements, and compareTo must be compatible with equals.
Rule name: NonComparableSetElements
Examples:
import java.util.*;
import org.apache.hc.client5.http.HttpRoute; // does implement equals/hashCode yet *not* compareTo
class Foo {
Set<String> strSet = new HashSet<>();
List<String> strList = new ArrayList<>();
Set<HttpRoute> fieldRouteSet = new HashSet<>(); // bad
List<HttpRoute> routeList = new ArrayList<>();
void byElemBad(Set<HttpRoute> paramRouteSet) { // bad
paramRouteSet.retainAll(routeList);
fieldRouteSet.contains(t);
Set<HttpRoute> localRouteSet = new HashSet<>(); // bad
localRouteSet.removeAll(routeList);
}
void otherCasesGood(Set<HttpRoute> paramRouteSet) {
strSet.contains("bla");
strSet.retainAll(strList);
strSet.remove("other");
paramRouteSet.iterator().next();
}
}See: Java 8 HashMap keys and Comparable
Note: In addition to the previous, this rule replaces Sonar rule java:S6411 The key type should implement Comparable, which has shortcomings.
Observation: A StringBuffer is used.
Problem: StringBuffer is thread-safe and has locking overhead.
Solution: Thread-safety not needed if only accessed by one thread. Use StringBuilder instead.
Rule name: AvoidStringBuffer.
Observation: Multiple statements are executed to concatenate to the same String with the +-operator. Example:
for (String val : values) {
logStatement += val;
}Problem: Each statement with one or more +-operators creates a hidden temporary StringBuilder, a char[] and a new String object, which all have to be garbage collected.
Solution: If you have multiple statements which concatenate to the same String with the +-operator in a method, it is more efficient to explicitly use a StringBuilder, append to it and use one toString at the exit of the method. This is especially true for concatenation within a loop.
Rule name: AvoidConcatInLoop, AvoidMultipleConcatStatements
Observation: A StringBuilder used to build long strings is constructed with default capacity.
Problem: The default initial capacity of StringBuilder is 16 characters. If more is appended, a new char[] will be allocated with: new-capacity = 2 * capacity + 2. The contents will be copied into the new char[] and the old char[] can be garbage collected. This allocation, copying and garbage collection takes time.
Solution: Construct the StringBuilder with an initial capacity explicitly:
StringBuilder builder = new StringBuilder(2048);Choose the value such that it will fit without resizing in most cases. And of course, you might want to define a constant field for the size.
Observation: Concatenation of Strings is used inside an StringBuilder.append argument.
Example:
builder.append("ExceptionType: " + ex.getClass().getName() + ", ");Problem: Concatenating one or more Strings creates a hidden temporary StringBuilder, a char[] and a new String object, which all have to be garbage collected. This generated StringBuilder is created in addition to the explicitly used StringBuilder.
Solution: Use one or more extra append on the explicit StringBuilder. Example:
builder.append("ExceptionType: ").append(ex.getClass().getName()).append(", ");Rule name: AvoidConcatInAppend
Observation: A StringBuilder is created and append is used for one statement resulting in a String.
Example:
public String bad() {
return new StringBuilder()
.append(name) // bad
.append(" = ")
.append(value)
.toString();
}Problem: Creating a StringBuilder and using append is more verbose, less readable and less maintainable than simply using String concatenation (+).
For one statement resulting in a String, creating a StringBuilder and using append is not faster than simply using concatenation.
Solution: Simply concatenate Strings in one statement, it is more concise, better readable and more maintainable. Example:
public String good() {
return name + " = " + value;
}Rule name: AvoidUnnecessaryStringBuilderCreation
Observation: A SimpleDateFormat is used.
Problem: java.util.SimpleDateFormat is thread-unsafe. The usual solution is to create a new one when needed in a method. Creating SimpleDateFormat is relatively expensive.
Solution: Use java.time.DateTimeFormatter. It is immutable, thus thread-safe and can be made static final. Example:
private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd");
// ...
LocalDateTime lastDate = DATE_FORMATTER.parse(lastDateString);Note: sometimes the use of SimpleDateFormat is forced by an API of a library. For instance, Jackson's ObjectMapper uses SimpleDateFormat. Because Jackson prevents threading issues, the rule allows the usage here. Beware there might be some contention when you use objectMapper.setDateFormat(SimpleDateFormat), if so, better create your own thread safe date serializers or use jackson-datatype-jsr310.
Like SimpleDateFormat, java.util.Date and -Calendar are mutable and flawed in other ways. Use Java 8+ java.time is the way to go.
Rule name: AvoidSimpleDateFormat.
Observation: A DateTimeFormatter is created from a pattern on every parse or print.
Problem: Recreating a DateTimeFormatter is relatively expensive.
Solution: java.time.DateTimeFormatter is thread-safe and can be shared among threads. Create the formatter from a pattern only once, to initialize a static field. See previous example.
Rule name: AvoidRecreatingDateTimeFormatter
Observation: Joda-time parseDateTime or printDateTime is used.
Problem: These methods consider timezone information which is expensive and usually unnecessary. There is a performance issue with time zones in joda time library 2.1-2.3
Solution: Use DateTimeFormatter.parseLocalDateTime(String) and DateTimeFormatter.print(LocalDateTime). In my benchmarks, the parse local is 2-3 times faster, and the print local is ~10% faster. Make sure the functionality is still correct. Note that LocalDateTime besides times zones does not support daylight saving time (DST).
Upgrade to version >= 2.4 or preferably, move to java.time.
Observation: One of the regular expression operations of String is used: matches, replaceAll, replaceFirst, split; or the static Pattern.matches().
Problem: A regular expression is implicitly compiled on every invocation, which can be expensive, depending on the length of the regular expression.
Solution: Use a matcher with the constant compiled regular expression pattern. Example:
old:
String getFileNameWithCount() {
return filename.replaceAll("(.\*)\\\\.(\[^\\\\.\]\*)", "$1\\\\(" + count + "\\\\).$2");
}new:
private static final Pattern AROUND_LAST_DOT_PATTERN = Pattern.compile("(.\*)\\\\.(\[^\\\\.\]\*)");
..
String getFileNameWithCount() {
return AROUND_LAST_DOT_PATTERN.matcher(filename).replaceAll("$1\\\\(" + count + "\\\\).$2");
}Rule name: AvoidImplicitlyRecompilingPatterns, improved: AvoidImplicitlyRecompilingRegex
Observation: A regular expression is compiled in a method.
Problem: A regular expression is compiled on every invocation, which can be expensive, depending on the length of the regular expression.
Solution: Compile the pattern only once and assign it to a private static field. java.util.Pattern objects are thread-safe so they can be shared among threads.
Rule name: AvoidRecompilingPatterns
Observation: java.net.URLEncoder.encode is called on an IBM JRE.
Problem: java.net.URLEncoder.encode is slow in multi-threaded environment for java.net.URLEncoder source before Java 6. This turned out to be a bottleneck in a large webshop application, I reported this in 2006 and it was fixed in Sun Java 6, see Sun Bug. However, IBM still uses a version of 17/11/2005 in JDK7 and even in JDK8, hence it suffers from this issue.
Solution: Use org.apache.commons.codec.net.URLCodec.encode instead.
Observation: Base64 encoding is achieved with sun.misc.BASE64Encoder and decoding with sun.misc.BASE64Decoder.
Problem: These implementations are not efficient
Solution: There is actually a hidden fast alternative since JAXB 1.0 / JavaEE 5+: javax.xml.bind.DatatypeConverter parseBase64Binary and printBase64Binary methods, see here. If you have Java 8+, use java.util.Base64, it is even a little faster.
Observation: An object field is mutable while it should not change after initialization, for a field like an ArrayList, HashMap, StringBuilder or a custom object.
Problem: The field can unintentionally be added to, so grow and become a memory leak.
Solution: Make the field immutable and final. In a constructor, defensively copy the modifiable argument, so also the caller is not able to modify the object referenced by the field anymore.
In case of an unmodifiable wrapped collection, make sure the inner collection is not directly reachable anymore after initialization.
Use e.g. for Lists: Java 9 List.of, Java 11 List.copyOf, Collections.unmodifiableList or Guava ImmutableList.
Rule name: AvoidMutableLists
Example:
class ConfBad {
private final List configItems; // bad, violation points here
public Conf(List listOfImmutableElems) {
configItems = new ArrayList(listOfImmutableElems); // can be mutated while not intended to be mutated
}
}
class ConfStillBad {
private List configItems = Collections.emptyList(); // bad, violation points here - immutable but re-assigned in constructor
public Conf(List listOfImmutableElems) {
configItems = new ArrayList(listOfImmutableElems); // can be mutated while not intended to be mutated
}
}
class ConfGood {
private final List configItems;
public Conf(List listOfImmutableElems) {
configItems = Collections.unmodifiableList(new ArrayList(listOfImmutableElems));
// configItems = List.copyOf(listOfImmutableElems) // Java 11
}
}
class PaymentUtil {
private static final Set<String> BRANCH_NAMES;
static {
final Set<String> branches = new HashSet<>();
branches.add("Company Antwerp Branch");
branches.add("Company Frankfurt Branch");
branches.add("Company London Branch");
BRANCH_NAMES = Collections.unmodifiableSet(branches);
// BRANCH_NAMES = Set.of(branches) // Java 9
}
}Observation: Timer objects must be shut-down properly on application shutdown/redeploy,
Problem: If not correctly shut down whole classloader remains in memory.
Solution: Implement an ... Aware bean and implement the OnDestroy ..
Observation: A Spring @ComponentScan or @EntityScan is set too wide eg. com.company.shop
Problem: Many unwanted and unexpected objects on the heap, wasting memory
Unexpected leaks could occur when initialized from a static context, which are unaware of lifecycle events.
Solution: Make your component scans as narrow as possible. You can also provide classes from the packages to scan, see example:
@EntityScan(basePackageClasses = {Job.class, ReportJob.class, ScheduleJob.class, Jsr310JpaConverters.class})Observation: Context and Dependency Injection is used with explicit calls of CDI.current().select(MyClass.class).get()
Problem: We found a memory leak with this use of CDI. Two objects of type com.ibm.ws.cdi.impl.CDIImpl appear in the heap. One is small and the other is very large. See and see more info: dont-get-trapped-into-a-memory-leak-using-cdi-instance-injection
Solution: use destroy to cleanup.
Example:
MyClass myObject= CDI.current().select(MyClass.class).get();
try {
....//do stuff
} finally {
CDI.current().destroy(myObject);
}Rule name: AvoidCDIReferenceLeak PMD-jPinpoint-rules/issues/28
Observation: A resilience4j retry event consumer is added to a retry event publisher for every method call. Likely a lambda retaining one or more objects.
Problem: This will result in a growing list of consumers: a memory leak. Besides, the event will be sent to the growing number of consumers, taking more and more CPU time.
Solution: Only call EventPublisher.onRetry (that is, add a consumer) in the same scope as the Retry instance lives. Note there is no way to unregister a consumer.
Example:
import io.github.resilience4j.retry.Retry;
public class Foo {
Retry retryField;
RetryRegistry reg;
final AtomicInteger retryCountField = new AtomicInteger();
Foo() {
retryField = reg.retry("one per service");
retryField.getEventPublisher().onRetry(event -> retryCountField.getAndIncrement()); // good
}
void callService() {
AtomicInteger retryCountLocal = new AtomicInteger();
retryField.getEventPublisher().onRetry(event -> retryCountLocal.getAndIncrement()); // bad, lambda and AtomicInt leak
Retry retryLocal = reg.retry("one per method call");
retryLocal.getEventPublisher().onRetry(event -> retryCountLocal.getAndIncrement()); // good, no leak
// same for onSuccess, onError, onIgnoredError
}
}Rule name: AvoidCDIReferenceLeak PMD-jPinpoint-rules/issues/28
Encapsulation (or information hiding), Don’t Repeat Yourself (DRY) and the Single Responsibility Principle (SRP) are important for performance because it makes it easier to improve performance by optimizing functionality, that is, the to-optimize code is limited to the implementation of a class.
Observation: Object exposes internal mutable state, a field like an ArrayList or a java.util.Date.
Problem: The state can be modified outside of the object, e.g. in case of a list it can be modified, added to, removed from or cleared, outside of object. No encapsulation.
Solution: If not really needed, remove the exposing method. Use proper encapsulation. Apply the object oriented: Tell, don’t ask principle: tell the object to do something with the state it owns, instead of asking for the internal state and do something with it outside of the object. If you think you have to expose the object, think again. If you then still have to expose, expose the object only unmodifyable using e.g. java.util.Collections.unmodifyableList() or create a copy in case of a mutable object like java.util.Date (or replace by an immutable type [java.time.LocalDateTime)
Observation: Object exposes internal unmodifyable data structure like an unmodifyable list.
Problem: The list is iterated over, some condition is tested and some operation is performed conditionally. This is actually a query, a selection of elements followed by an operation. Quite similar of these queries+operations typically exist in various places.
Solution: See previous item, apply: Tell, don't ask. See also Violation of encapsulation. These queries+operations are much easier to optimize if they are located in one place.
Observation: A mutable static field is used. This can be a static non-final reference or a static final mutable object.
Problem: Violates encapsulation, the state can be modified outside of the object/class. If modified and used by multiple threads, it is thread-unsafe, may cause corruption of data or not viewing changes done by other threads.
Solution: Make it a final reference, possibly a blank final; and/or an immutable object or unmodifiable via a wrapper like Collections.unmodifiableList.
Observation: An interface is used to define constants.
Problem: Constants are often implementation details. Putting constants in an interface makes them part of the public API of all implementing classes. Doing this for constants which are implementation details is bad OO practice. It is a documented anti-pattern.
Solution: Make it a Class which cannot be instantiated, or an Enum. Use static imports.
Rule name: AvoidConstantsInInterface.
Observation: A constructor or setter shares internal state. A reference to a mutable object is passed in a constructor or setter and assigned to a field, it is used directly as state of the object.
Problem: Internal state can be modified from outside of the object, by using the reference to the mutable object by the caller of the constructor or setter. Risk of thread-unsafety.
Solution:
- Defensively copy the referenced object such that no mutable state is shared.
- Use a builder pattern in case of more than a few parameters.
Observation: A Java record shares internal mutable state. Records are a concise way to code your plain "data carriers". They are transparent holders for shallowly immutable data, that is, the fields (references) are final.
Still, fields may reference mutable objects.
Problem: Internal state can be modified from outside of the record, through the implicit accessor method or by the caller of the constructor. Risk of thread-unsafety.
Solution: Use record compact constructor to defensively copy the (possibly) mutable object such as a List, Set or Map to make the record deeply immutable.
Note that for this the collection elements also need to be deeply immutable. Note that copyOf does not accept a null collection nor null elements.
Rule name: AvoidExposingMutableRecordState
Example:
record BadRecord(String name, List<String> list) { // bad, possibly mutable List exposed
}
record GoodRecord(String name, @NonNull List<String> list) {
public GoodRecord {
list = List.copyOf(list); // good, immutable list
}
}
record GoodRecordNullable(String name, @Nullable List<String> list) {
public GoodRecordNullable {
if (list != null) {
list = List.copyOf(list);
}
}
}Note: copyOf() doesn't actually do any copying if the input is already an immutable list created via List.of() (or related methods).
See: enforce-immutable-collections-in-a-java-record and jep-395.