Skip to content

Commit

Permalink
Optimize finding the file to replace when handling an upload
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
michaelhixson committed Apr 9, 2019
1 parent c31dafc commit 4763e10
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 33 deletions.
40 changes: 25 additions & 15 deletions src/main/java/tfb/status/handler/UploadResultsHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -70,6 +71,7 @@ public UploadResultsHandler(FileStore fileStore,
ObjectMapper objectMapper,
EmailSender emailSender,
HomeUpdatesHandler homeUpdates,
HomeResultsReader homeResultsReader,
DiffGenerator diffGenerator,
Clock clock,
RunProgressMonitor runProgressMonitor) {
Expand All @@ -79,6 +81,7 @@ public UploadResultsHandler(FileStore fileStore,
/* fileStore= */ fileStore,
/* authenticator= */ authenticator,
/* homeUpdates=*/ homeUpdates,
/* homeResultsReader=*/ homeResultsReader,
/* clock= */ clock,
/* objectMapper=*/ objectMapper,
/* runProgressMonitor= */ runProgressMonitor);
Expand All @@ -88,6 +91,7 @@ public UploadResultsHandler(FileStore fileStore,
/* fileStore= */ fileStore,
/* authenticator= */ authenticator,
/* homeUpdates=*/ homeUpdates,
/* homeResultsReader=*/ homeResultsReader,
/* clock= */ clock,
/* objectMapper=*/ objectMapper,
/* emailSender=*/ emailSender,
Expand Down Expand Up @@ -115,17 +119,20 @@ 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;

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);
Expand Down Expand Up @@ -176,18 +183,17 @@ private Path destinationForIncomingFile(Path incomingFile)
if (incomingUuid == null)
return newResultsFile();

try (DirectoryStream<Path> 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();
}
Expand Down Expand Up @@ -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) {
Expand All @@ -246,6 +253,7 @@ private static final class JsonHandler extends BaseHandler {
/* fileStore= */ fileStore,
/* authenticator= */ authenticator,
/* homeUpdates= */ homeUpdates,
/* homeResultsReader= */ homeResultsReader,
/* clock= */ clock,
/* fileExtension= */ "json");

Expand All @@ -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;
}
Expand Down Expand Up @@ -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,
Expand All @@ -324,6 +333,7 @@ private static final class ZipHandler extends BaseHandler {
/* fileStore= */ fileStore,
/* authenticator= */ authenticator,
/* homeUpdates= */ homeUpdates,
/* homeResultsReader= */ homeResultsReader,
/* clock= */ clock,
/* fileExtension= */ "zip");

Expand All @@ -339,15 +349,15 @@ private static final class ZipHandler extends BaseHandler {
@Nullable
String tryReadUuid(Path zipFile) {
Objects.requireNonNull(zipFile);
UuidOnly parsed;
Results parsed;
try {
parsed =
ZipFiles.readZipEntry(
/* zipFile= */ zipFile,
/* entryPath= */ "results.json",
/* entryReader= */ inputStream ->
objectMapper.readValue(inputStream,
UuidOnly.class));
Results.class));
} catch (IOException ignored) {
return null;
}
Expand Down
18 changes: 0 additions & 18 deletions src/main/java/tfb/status/view/Results.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down

0 comments on commit 4763e10

Please sign in to comment.