mirror of
https://github.com/spring-projects/spring-boot.git
synced 2026-09-30 06:09:14 +00:00
Check resource existence once when looking for deleted files
ClassLoaderFilesResourcePatternResolver.isDeleted() calls resource.exists() and resource.getURI() inside the loop over the uploaded files, so both are repeated once per DELETED entry although neither depends on the entry. Every resource lookup made while the application context is being built goes through this method - about 1300 times per restart in the application I measured - so a session that has accumulated 100 deleted files performs around 130 000 file system lookups where 1300 are enough. This commit hoists both calls out of the loop: the resource is inspected at most once per call, on the first DELETED entry, and the URI comparison then uses the cached value. The method still returns on the first match and still reports a failing getURI() as an IllegalStateException, so behaviour is unchanged. The loop now walks the per-directory entry sets, which is what ClassLoaderFiles.addAll() and RestartServer already do, instead of the flattened view added in gh-46289. That view is wrapped in Collections.unmodifiableSet(), whose iterator is shared JDK code; in a running application its delegate calls are megamorphic and C2 stops inlining them, which made restarts with many accumulated files and no deletions slower than before gh-46289. isDeleted() returns a boolean, so the visiting order of the entries cannot change its result. Measured on a Spring Boot application driven through the remote restart path (restart request to ApplicationReadyEvent), 12 JVMs per variant and 20 restarts per JVM, medians: 100 source directories, 10 000 entries, 10 deleted: 278 ms -> 200 ms 100 source directories, 10 000 entries, 100 deleted: 891 ms -> 201 ms 100 source directories, 10 000 entries, 500 deleted: 3536 ms -> 200 ms 100 source directories, 50 000 entries, none deleted: 431 ms -> 341 ms 100 source directories, 10 000 entries, none deleted: 202 ms -> 192 ms no uploaded files: 150 ms -> 148 ms See gh-51841 Signed-off-by: DongHoon Lee <dhl1924@naver.com>
This commit is contained in:
committed by
Stéphane Nicoll
parent
76cc145900
commit
5c2ad14f50
+24
-9
@@ -33,6 +33,7 @@ import org.springframework.boot.devtools.restart.classloader.ClassLoaderFile;
|
||||
import org.springframework.boot.devtools.restart.classloader.ClassLoaderFile.Kind;
|
||||
import org.springframework.boot.devtools.restart.classloader.ClassLoaderFileURLStreamHandler;
|
||||
import org.springframework.boot.devtools.restart.classloader.ClassLoaderFiles;
|
||||
import org.springframework.boot.devtools.restart.classloader.ClassLoaderFiles.SourceDirectory;
|
||||
import org.springframework.context.ApplicationContext;
|
||||
import org.springframework.context.support.AbstractApplicationContext;
|
||||
import org.springframework.core.io.AbstractResource;
|
||||
@@ -58,6 +59,7 @@ import org.springframework.web.context.support.ServletContextResourcePatternReso
|
||||
* @author Andy Wilkinson
|
||||
* @author Phillip Webb
|
||||
* @author Stephane Nicoll
|
||||
* @author DongHoon Lee
|
||||
*/
|
||||
final class ClassLoaderFilesResourcePatternResolver implements ResourcePatternResolver {
|
||||
|
||||
@@ -146,22 +148,35 @@ final class ClassLoaderFilesResourcePatternResolver implements ResourcePatternRe
|
||||
}
|
||||
|
||||
private boolean isDeleted(Resource resource) {
|
||||
for (Entry<String, ClassLoaderFile> entry : this.classLoaderFiles.getFileEntries()) {
|
||||
try {
|
||||
String name = entry.getKey();
|
||||
ClassLoaderFile file = entry.getValue();
|
||||
if (file.getKind() == Kind.DELETED && resource.exists()
|
||||
&& resource.getURI().toString().endsWith(name)) {
|
||||
String uri = null;
|
||||
for (SourceDirectory sourceDirectory : this.classLoaderFiles.getSourceDirectories()) {
|
||||
for (Entry<String, ClassLoaderFile> entry : sourceDirectory.getFilesEntrySet()) {
|
||||
if (entry.getValue().getKind() != Kind.DELETED) {
|
||||
continue;
|
||||
}
|
||||
if (uri == null) {
|
||||
if (!resource.exists()) {
|
||||
return false;
|
||||
}
|
||||
uri = getUri(resource);
|
||||
}
|
||||
if (uri.endsWith(entry.getKey())) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
catch (IOException ex) {
|
||||
throw new IllegalStateException("Failed to retrieve URI from '" + resource + "'", ex);
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
private String getUri(Resource resource) {
|
||||
try {
|
||||
return resource.getURI().toString();
|
||||
}
|
||||
catch (IOException ex) {
|
||||
throw new IllegalStateException("Failed to retrieve URI from '" + resource + "'", ex);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A {@link Resource} that represents a {@link ClassLoaderFile} that has been
|
||||
* {@link Kind#DELETED deleted}.
|
||||
|
||||
+44
@@ -18,6 +18,7 @@ package org.springframework.boot.devtools.restart;
|
||||
|
||||
import java.io.File;
|
||||
import java.io.IOException;
|
||||
import java.net.URI;
|
||||
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
@@ -43,6 +44,7 @@ import static org.mockito.ArgumentMatchers.eq;
|
||||
import static org.mockito.BDDMockito.given;
|
||||
import static org.mockito.BDDMockito.then;
|
||||
import static org.mockito.Mockito.mock;
|
||||
import static org.mockito.Mockito.never;
|
||||
|
||||
/**
|
||||
* Tests for {@link ClassLoaderFilesResourcePatternResolver}.
|
||||
@@ -50,6 +52,7 @@ import static org.mockito.Mockito.mock;
|
||||
* @author Phillip Webb
|
||||
* @author Andy Wilkinson
|
||||
* @author Stephane Nicoll
|
||||
* @author DongHoon Lee
|
||||
*/
|
||||
class ClassLoaderFilesResourcePatternResolverTests {
|
||||
|
||||
@@ -90,6 +93,41 @@ class ClassLoaderFilesResourcePatternResolverTests {
|
||||
assertThat(resource).isInstanceOf(DeletedClassLoaderFileResource.class);
|
||||
}
|
||||
|
||||
@Test
|
||||
void getResourceWhenDeletedInAnotherSourceDirectoryShouldReturnDeletedResource(@TempDir File directory)
|
||||
throws Exception {
|
||||
File file = createFile(directory, "name.class");
|
||||
this.files.addFile("one", "other.class", new ClassLoaderFile(Kind.ADDED, new byte[0]));
|
||||
this.files.addFile("two", "name.class", new ClassLoaderFile(Kind.DELETED, null));
|
||||
Resource resource = this.resolver.getResource("file:" + file.getAbsolutePath());
|
||||
assertThat(resource).isInstanceOf(DeletedClassLoaderFileResource.class);
|
||||
}
|
||||
|
||||
@Test
|
||||
void getResourceWhenManyFilesAreDeletedShouldCheckResourceOnlyOnce() throws Exception {
|
||||
Resource resource = mock(Resource.class);
|
||||
given(resource.exists()).willReturn(true);
|
||||
given(resource.getURI()).willReturn(URI.create("file:/app/classes/three.class"));
|
||||
this.resolver = createResolverResolving("foo:some-file.txt", resource);
|
||||
this.files.addFile("one", "one.class", new ClassLoaderFile(Kind.DELETED, null));
|
||||
this.files.addFile("one", "two.class", new ClassLoaderFile(Kind.DELETED, null));
|
||||
this.files.addFile("two", "three.class", new ClassLoaderFile(Kind.DELETED, null));
|
||||
assertThat(this.resolver.getResource("foo:some-file.txt")).isInstanceOf(DeletedClassLoaderFileResource.class);
|
||||
then(resource).should().exists();
|
||||
then(resource).should().getURI();
|
||||
}
|
||||
|
||||
@Test
|
||||
void getResourceWhenNoFileIsDeletedShouldNotCheckResource() throws Exception {
|
||||
Resource resource = mock(Resource.class);
|
||||
this.resolver = createResolverResolving("foo:some-file.txt", resource);
|
||||
this.files.addFile("one", "one.class", new ClassLoaderFile(Kind.ADDED, new byte[0]));
|
||||
this.files.addFile("two", "two.class", new ClassLoaderFile(Kind.MODIFIED, new byte[0]));
|
||||
assertThat(this.resolver.getResource("foo:some-file.txt")).isSameAs(resource);
|
||||
then(resource).should(never()).exists();
|
||||
then(resource).should(never()).getURI();
|
||||
}
|
||||
|
||||
@Test
|
||||
void getResourcesShouldReturnResources(@TempDir File directory) throws Exception {
|
||||
File file = createFile(directory, "name.class");
|
||||
@@ -173,6 +211,12 @@ class ClassLoaderFilesResourcePatternResolverTests {
|
||||
then(resolver).should().resolve(eq("foo:some-file.txt"), any(ResourceLoader.class));
|
||||
}
|
||||
|
||||
private ClassLoaderFilesResourcePatternResolver createResolverResolving(String location, Resource resource) {
|
||||
GenericApplicationContext context = new GenericApplicationContext();
|
||||
context.addProtocolResolver(mockProtocolResolver(location, resource));
|
||||
return new ClassLoaderFilesResourcePatternResolver(context, this.files);
|
||||
}
|
||||
|
||||
private ProtocolResolver mockProtocolResolver(String path, Resource resource) {
|
||||
ProtocolResolver resolver = mock(ProtocolResolver.class);
|
||||
given(resolver.resolve(eq(path), any(ResourceLoader.class))).willReturn(resource);
|
||||
|
||||
Reference in New Issue
Block a user