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

[qq2357509207]

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

synchronized seriously affects the performance of executing redis lua scripts
Usually, the redis lua script will not change

[mp911de]

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:

spring-data-redis/src/main/java/org/springframework/data/redis/core/script/DefaultRedisScript.java

Lines 85 to 93 in b57a1e0

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

[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.

[rawalprafull]

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

[mp911de]

Sure, feel free.

[rawalprafull]

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..

[mp911de]

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

[psw0946]

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
Even when limiting the synchronized block to the sha1 assignment, scriptSource.isModified() method is also executed in synchronized block both in StaticScriptSource and ResourceScriptSource. It would be the bottleneck in high TPS multi-thread environment.
As the reporter said, the redis lua script will not change during runtime in most cases, I think.
How about creating new ScriptSource like UnmodifiableScriptSource in this project for those cases?

[psw0946]

Or we can just create new RedisScript implementing RedisScript<T> or extending DefaultRedisScript, and override getSha1() method without checking scriptSource.isModified.