spring-data-redis icon indicating copy to clipboard operation
spring-data-redis copied to clipboard

`DefaultRedisScript` checks for `isModified` in a `synchronized` block that affects multi-threaded performance

Open qq2357509207 opened this issue 4 years ago • 2 comments

org.springframework.data.redis.core.script.DefaultRedisScript

Snipaste_2021-07-21_15-08-14 synchronized seriously affects the performance of executing redis lua scripts Usually, the redis lua script will not change

qq2357509207 avatar Jul 21 '21 07:07 qq2357509207

Please refrain from pasting images into bug reports as these cannot be searched or indexed. Instead, use stable links to code such as the following one:

https://github.com/spring-projects/spring-data-redis/blob/b57a1e0376a55c42eb3f8be5cc1a6ebfe7b33afc/src/main/java/org/springframework/data/redis/core/script/DefaultRedisScript.java#L85-L93

mp911de avatar Jul 21 '21 07:07 mp911de

The code could be improved by limiting the synchronized block to the sha1 assignment. In return, the sha1 field can become subject to visibility issues across multiple threads which can lead to multiple computations of the SHA value but that is the flip side of best effort caching.

Feel free to submit a pull request.

mp911de avatar Jul 21 '21 07:07 mp911de

@mp911de can I contribute in this to kick start my journey ? if no one is working on this.

rawalprafull avatar Aug 18 '23 20:08 rawalprafull

Sure, feel free.

mp911de avatar Aug 22 '23 14:08 mp911de

public String getSha1() {
	if (sha1 == null || scriptSource.isModified()) {
		synchronized (shaModifiedMonitor) {
			this.sha1 = DigestUtils.sha1DigestAsHex(getScriptAsString());
		}
	}
	return sha1;
}

private @Nullable ScriptSource scriptSource; // line no. 42

@mp911de method invocation scriptSource.isModified() may produce NullPointerException. we need to handle this too, please correct me if wrong here..

rawalprafull avatar Aug 22 '23 20:08 rawalprafull

DefaultRedisScript is a bean that incorporates lifecycle via InitializingBean. I took a look at the problem and introduced a general revision of all synchronized blocks that lead to blocking calls via #2691

mp911de avatar Aug 23 '23 12:08 mp911de