Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(http/file_server): read fileinfo in parallel #3363

Merged
merged 2 commits into from
May 6, 2023

Conversation

ayame113
Copy link
Contributor

@ayame113 ayame113 commented May 5, 2023

part of #3361

Previously, fileInfo was read serially when generating directory listing pages. Change this to read in parallel.
After this PR, the display of the directory listing (page below) is tens of milliseconds faster.

image

// When there are 100 files in the directory:
benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
before     29.6 ms/iter (20.56 ms … 34.89 ms) 31.05 ms 34.89 ms 34.89 ms
after      4.27 ms/iter   (3.51 ms … 6.63 ms)  4.44 ms  5.27 ms  6.63 ms

// When there are 1000 files in the directory:
benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
before     310.22 ms/iter (295.08 ms … 331.81 ms) 317.34 ms 331.81 ms 331.81 ms
after       30.35 ms/iter   (28.24 ms … 32.86 ms)  31.65 ms  32.86 ms  32.86 ms
benchmark code:
// I created 100 files in the testdata/bench directory and run the `deno bench` command.

import { serveDir } from "./file_server.ts";
import { serveDir as serveDirOld } from "./file_server_old.ts";

Deno.bench(async function after() {
  const res = await serveDir(
    new Request("http://localhost:4507/http/testdata/bench/"),
    { quiet: true, showDirListing: true },
  );
  await res.text();
});

Deno.bench(async function before() {
  const res = await serveDirOld(
    new Request("http://localhost:4507/http/testdata/bench/"),
    { quiet: true, showDirListing: true },
  );
  await res.text();
});

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -255,7 +253,6 @@ export async function serveFile(
return createCommonResponse(Status.OK, file.readable, { headers });
}

// TODO(bartlomieju): simplify this after deno.stat and deno.readDir are fixed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This is an ancient comment by Kevin and this doesn't seem relevant anymore. https://github.com/denoland/deno_std/pull/15/files#diff-106086daad2313990285a9c61085eeb5995d479127db151e613f84ea709450c8R104

@@ -23,8 +23,6 @@ interface EntryInfo {
name: string;
}

const encoder = new TextEncoder();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Response object take care of encoding, and this is unnecessary now. Nice clean up.

@kt3k kt3k merged commit 163a902 into denoland:main May 6, 2023
@ayame113 ayame113 deleted the file-server-readdir-parallel branch May 6, 2023 10:11
@ayame113
Copy link
Contributor Author

ayame113 commented May 6, 2023

Thank you for your review! 🙇‍♂️

There is a PR on deno_blog that does a similar optimization. If you don't mind, please take a look when you have time. denoland/deno_blog#116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants