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

feat(serve-static): add precompressed option #199

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

yusukebe
Copy link
Member

Implemented the same feature of honojs/hono#3366

@yusukebe
Copy link
Member Author

yusukebe commented Sep 15, 2024

Hey @usualoma !

Can you review this?

@@ -199,7 +199,7 @@ app.use(
You can specify handling when the requested file is found with `onFound`.

```ts
app.get(
app.use(
Copy link
Member Author

Choose a reason for hiding this comment

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

Other examples are using use.

@usualoma
Copy link
Member

I'll check it out, just give me a minute.

@usualoma
Copy link
Member

Sorry I haven't commented on this at honojs/hono#3366. I think it is a very good feature, but the implementation could use some improvement in my opinion.

I have created #200 and would like you to consider merging

calling sort or Object.keys

It would be better to avoid calling sort or Object.keys on every request.

In my environment, I got the following results

import { run, bench, group } from "mitata";

const ENCODINGS = {
  br: ".br",
  zstd: ".zst",
  gzip: ".gz",
} as const;
const ENCODINGS_ORDERED_KEYS = Object.keys(ENCODINGS) as (keyof typeof ENCODINGS)[]

const filterAndSort = (ae: string) => {
  const acceptEncodings =
    ae
      .split(",")
      .map((encoding) => encoding.trim())
      .filter((encoding): encoding is keyof typeof ENCODINGS =>
        Object.hasOwn(ENCODINGS, encoding)
      )
      .sort(
        (a, b) =>
          Object.keys(ENCODINGS).indexOf(a) - Object.keys(ENCODINGS).indexOf(b)
      ) ?? [];

  for (const encoding of acceptEncodings) {
    break;
  }
};

const set = (ae: string) => {
  const acceptEncodingsSet = new Set(ae.split(",").map((encoding) => encoding.trim()));

  for (const encoding of ENCODINGS_ORDERED_KEYS) {
    if (!acceptEncodingsSet.has(encoding)) {
      continue;
    }
    break;
  }
};

group("filterAndSort vs set", () => {
  bench("filterAndSort", () => filterAndSort("br,gzip,zstd"));
  bench("set", () => set("br,gzip,zstd"));
});

run();
cpu: Apple M2 Pro
runtime: node v20.14.0 (arm64-darwin)

benchmark          time (avg)             (min … max)       p75       p99      p999
----------------------------------------------------- -----------------------------
• filterAndSort vs set
----------------------------------------------------- -----------------------------
filterAndSort     284 ns/iter       (258 ns … 562 ns)    299 ns    357 ns    498 ns
set               127 ns/iter       (118 ns … 294 ns)    126 ns    178 ns    236 ns

summary for filterAndSort vs set
  set
   2.24x faster than filterAndSort

Find files only if mime type is compressible

If all that is delivered from static is css or js, it would be a useless check, but image files are often placed in static and may be files that are not compressed additionally. Access to the file system is generally very heavy, so it would be better to check the mime type and look for it only when the file should be compressed.

@yusukebe
Copy link
Member Author

@usualoma Thanks! I'll check it later.

@yusukebe
Copy link
Member Author

Hi @usualoma

Fully agree with your improvements. Using Set is fast, and choosing a compressible file is good! I'll merge your branch into this. Thank you!

#200)

* perf(serve-static): use "Set" for checking precompressed

* perf(serve-static): search precompressed file only if content type is compressible
@yusukebe yusukebe merged commit 98d711a into main Sep 16, 2024
3 checks passed
@yusukebe yusukebe deleted the feat/serve-static-precompressed branch September 16, 2024 03:39
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