From 4763e10410a2d2d55d6a8259e2bc274c51398a49 Mon Sep 17 00:00:00 2001 From: Michael Hixson Date: Mon, 8 Apr 2019 17:10:43 -0700 Subject: [PATCH] Optimize finding the file to replace when handling an upload Before, we would parse every existing results file of the same type to find the one with the same uuid. We had the `UuidOnly` optimization to make that parsing 2/3 as expensive as parsing a full `Results` object, but it was still slow. Now, we make use of HomeResultsReader's cache, which already associates uuids with file names. Since we're no longer calling `tryReadUuid` for a large number of files within the scope of each request, there is much less benefit to micro-optimizing `tryReadUuid`. The only reason `UuidOnly` exists is to micro-optimize `tryReadUuid`, so we can get rid of `UuidOnly` now. --- .../status/handler/UploadResultsHandler.java | 40 ++++++++++++------- src/main/java/tfb/status/view/Results.java | 18 --------- 2 files changed, 25 insertions(+), 33 deletions(-) diff --git a/src/main/java/tfb/status/handler/UploadResultsHandler.java b/src/main/java/tfb/status/handler/UploadResultsHandler.java index 34d89035..c3c41f53 100644 --- a/src/main/java/tfb/status/handler/UploadResultsHandler.java +++ b/src/main/java/tfb/status/handler/UploadResultsHandler.java @@ -46,12 +46,13 @@ import tfb.status.service.DiffGenerator; import tfb.status.service.EmailSender; import tfb.status.service.FileStore; +import tfb.status.service.HomeResultsReader; import tfb.status.service.RunProgressMonitor; import tfb.status.undertow.extensions.MediaTypeHandler; import tfb.status.undertow.extensions.MethodHandler; import tfb.status.util.ZipFiles; +import tfb.status.view.HomePageView.ResultsView; import tfb.status.view.Results; -import tfb.status.view.Results.UuidOnly; /** * Handles requests to upload a file containing results from a TFB run. The @@ -70,6 +71,7 @@ public UploadResultsHandler(FileStore fileStore, ObjectMapper objectMapper, EmailSender emailSender, HomeUpdatesHandler homeUpdates, + HomeResultsReader homeResultsReader, DiffGenerator diffGenerator, Clock clock, RunProgressMonitor runProgressMonitor) { @@ -79,6 +81,7 @@ public UploadResultsHandler(FileStore fileStore, /* fileStore= */ fileStore, /* authenticator= */ authenticator, /* homeUpdates=*/ homeUpdates, + /* homeResultsReader=*/ homeResultsReader, /* clock= */ clock, /* objectMapper=*/ objectMapper, /* runProgressMonitor= */ runProgressMonitor); @@ -88,6 +91,7 @@ public UploadResultsHandler(FileStore fileStore, /* fileStore= */ fileStore, /* authenticator= */ authenticator, /* homeUpdates=*/ homeUpdates, + /* homeResultsReader=*/ homeResultsReader, /* clock= */ clock, /* objectMapper=*/ objectMapper, /* emailSender=*/ emailSender, @@ -115,6 +119,7 @@ public void handleRequest(HttpServerExchange exchange) throws Exception { */ private abstract static class BaseHandler implements HttpHandler { private final HomeUpdatesHandler homeUpdates; + private final HomeResultsReader homeResultsReader; private final Clock clock; private final FileStore fileStore; private final String fileExtension; @@ -122,10 +127,12 @@ private abstract static class BaseHandler implements HttpHandler { BaseHandler(FileStore fileStore, Authenticator authenticator, HomeUpdatesHandler homeUpdates, + HomeResultsReader homeResultsReader, Clock clock, String fileExtension) { this.homeUpdates = Objects.requireNonNull(homeUpdates); + this.homeResultsReader = Objects.requireNonNull(homeResultsReader); this.clock = Objects.requireNonNull(clock); this.fileStore = Objects.requireNonNull(fileStore); this.fileExtension = Objects.requireNonNull(fileExtension); @@ -176,18 +183,17 @@ private Path destinationForIncomingFile(Path incomingFile) if (incomingUuid == null) return newResultsFile(); - try (DirectoryStream filesOfSameType = - Files.newDirectoryStream(fileStore.resultsDirectory(), - "*." + fileExtension)) { + ResultsView results = homeResultsReader.resultsByUuid(incomingUuid); + if (results == null) + return newResultsFile(); - for (Path existingFile : filesOfSameType) { - String existingUuid = tryReadUuid(existingFile); + if (results.json != null + && results.json.fileName.endsWith("." + fileExtension)) + return fileStore.resultsDirectory().resolve(results.json.fileName); - // TODO: Also check if the file was updated more recently than ours? - if (incomingUuid.equals(existingUuid)) - return existingFile; - } - } + if (results.zip != null + && results.zip.fileName.endsWith("." + fileExtension)) + return fileStore.resultsDirectory().resolve(results.zip.fileName); return newResultsFile(); } @@ -238,6 +244,7 @@ private static final class JsonHandler extends BaseHandler { JsonHandler(FileStore fileStore, Authenticator authenticator, HomeUpdatesHandler homeUpdates, + HomeResultsReader homeResultsReader, Clock clock, ObjectMapper objectMapper, RunProgressMonitor runProgressMonitor) { @@ -246,6 +253,7 @@ private static final class JsonHandler extends BaseHandler { /* fileStore= */ fileStore, /* authenticator= */ authenticator, /* homeUpdates= */ homeUpdates, + /* homeResultsReader= */ homeResultsReader, /* clock= */ clock, /* fileExtension= */ "json"); @@ -257,9 +265,9 @@ private static final class JsonHandler extends BaseHandler { @Nullable String tryReadUuid(Path jsonFile) { Objects.requireNonNull(jsonFile); - UuidOnly parsed; + Results parsed; try (InputStream inputStream = Files.newInputStream(jsonFile)) { - parsed = objectMapper.readValue(inputStream, UuidOnly.class); + parsed = objectMapper.readValue(inputStream, Results.class); } catch (IOException ignored) { return null; } @@ -314,6 +322,7 @@ private static final class ZipHandler extends BaseHandler { ZipHandler(FileStore fileStore, Authenticator authenticator, HomeUpdatesHandler homeUpdates, + HomeResultsReader homeResultsReader, Clock clock, ObjectMapper objectMapper, EmailSender emailSender, @@ -324,6 +333,7 @@ private static final class ZipHandler extends BaseHandler { /* fileStore= */ fileStore, /* authenticator= */ authenticator, /* homeUpdates= */ homeUpdates, + /* homeResultsReader= */ homeResultsReader, /* clock= */ clock, /* fileExtension= */ "zip"); @@ -339,7 +349,7 @@ private static final class ZipHandler extends BaseHandler { @Nullable String tryReadUuid(Path zipFile) { Objects.requireNonNull(zipFile); - UuidOnly parsed; + Results parsed; try { parsed = ZipFiles.readZipEntry( @@ -347,7 +357,7 @@ String tryReadUuid(Path zipFile) { /* entryPath= */ "results.json", /* entryReader= */ inputStream -> objectMapper.readValue(inputStream, - UuidOnly.class)); + Results.class)); } catch (IOException ignored) { return null; } diff --git a/src/main/java/tfb/status/view/Results.java b/src/main/java/tfb/status/view/Results.java index 59cebc76..cf0d979c 100644 --- a/src/main/java/tfb/status/view/Results.java +++ b/src/main/java/tfb/status/view/Results.java @@ -473,24 +473,6 @@ public TfbWebsiteView( } } - /** - * A slimmed-down view of a results.json file that only includes its - * (optional) {@link Results#uuid} field. - */ - @Immutable - public static final class UuidOnly { - /** - * See {@link Results#uuid}. - */ - @Nullable - public final String uuid; - - @JsonCreator - public UuidOnly(@Nullable @JsonProperty("uuid") String uuid) { - this.uuid = uuid; - } - } - /** * The set of all known test types. */