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): optimize file server #2116

Merged
merged 6 commits into from
Apr 18, 2022

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Apr 16, 2022

Towards #2107

Major changes:

  • use native readable streams optimization. Deno.File#readable instead of a wrapper stream. See perf(http): optimize ReadableStreams backed by a resource deno#14284
  • use fnv-1a default etag hash function. Adds an option to allow using others.
  • faster hex function for out of other algorithms (like sha-1), using hex lookup table.
  • disables logging by default when invoked independently. Replaces the --quiet flag with --verbose. Public API remains unchanged.
  • use simpler extname extraction to avoid scanning manually. (downsides?)

Benchmarks:

main:

Running 10s test @ http:https://localhost:4507/README.md
  8 threads and 20 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.44ms  522.77us  26.73ms   97.55%
    Req/Sec     1.40k   119.07     2.63k    97.39%
  112288 requests in 10.10s, 432.42MB read
Requests/sec:  11114.28
Transfer/sec:     42.80MB

This patch:

wrk -t8 -c20 -d10s http:https://localhost:4507/README.md
Running 10s test @ http:https://localhost:4507/README.md
  8 threads and 20 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     1.01ms  163.42us   6.32ms   85.73%
    Req/Sec     1.99k    67.51     2.09k    86.51%
  160068 requests in 10.10s, 611.53MB read
Requests/sec:  15848.72
Transfer/sec:     60.55MB

@littledivy littledivy marked this pull request as draft April 16, 2022 10:55
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Great that you have managed to optimize this. This is code we want people to read and understand though, so we shouldn't duplicate functionality from other std/ modules.

http/file_server.ts Outdated Show resolved Hide resolved
http/file_server.ts Outdated Show resolved Hide resolved
Comment on lines +196 to +208
// The fnv-1a hash function.
function fnv1a(buf: string): string {
let hash = 2166136261; // 32-bit FNV offset basis
for (let i = 0; i < buf.length; i++) {
hash ^= buf.charCodeAt(i);
// Equivalent to `hash *= 16777619` without using BigInt
// 32-bit FNV prime
hash += (hash << 1) + (hash << 4) + (hash << 7) + (hash << 8) +
(hash << 24);
}
// 32-bit hex string
return (hash >>> 0).toString(16);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this not available is std/crypto?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, can't find it

http/file_server.ts Outdated Show resolved Hide resolved
Comment on lines -100 to +102
diffMessages.push(c(`${createSign(result.type)}${line}`));
diffMessages.push(c(`${
createSign(result.type)
}${line}`));
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change?

Copy link
Member

Choose a reason for hiding this comment

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

I've seen this change recently in some other PR, seems to be related to dprint. CC @dsherret

@littledivy littledivy marked this pull request as ready for review April 16, 2022 13:08
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

http/file_server.ts Outdated Show resolved Hide resolved
http/file_server.ts Outdated Show resolved Hide resolved
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

None yet

3 participants