Skip to content

Optimize HTTP/2 request creation #423

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 28 additions & 13 deletions lib/mint/http2.ex
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ defmodule Mint.HTTP2 do
:hostname,
:port,
:scheme,
:authority,

# Connection state (open, closed, and so on).
:state,
Expand Down Expand Up @@ -974,10 +975,18 @@ defmodule Mint.HTTP2 do
) :: {:ok, t()} | {:error, Types.error()}
def initiate(scheme, socket, hostname, port, opts) do
transport = scheme_to_transport(scheme)
scheme_string = Atom.to_string(scheme)
mode = Keyword.get(opts, :mode, :active)
log? = Keyword.get(opts, :log, false)
client_settings_params = Keyword.get(opts, :client_settings, [])
validate_client_settings!(client_settings_params)
# If the port is the default for the scheme, don't add it to the :authority pseudo-header
authority =
if URI.default_port(scheme_string) == port do
hostname
else
"#{hostname}:#{port}"
end

unless mode in [:active, :passive] do
raise ArgumentError,
Expand All @@ -992,10 +1001,11 @@ defmodule Mint.HTTP2 do
conn = %__MODULE__{
hostname: hostname,
port: port,
authority: authority,
transport: scheme_to_transport(scheme),
socket: socket,
mode: mode,
scheme: Atom.to_string(scheme),
scheme: scheme_string,
state: :handshaking,
log: log?
}
Expand Down Expand Up @@ -1355,23 +1365,37 @@ defmodule Mint.HTTP2 do
end

defp add_pseudo_headers(headers, conn, method, path) do
if String.upcase(method) == "CONNECT" do
if is_method?(method, ~c"CONNECT") do
[
{":method", method},
{":authority", authority_pseudo_header(conn.scheme, conn.port, conn.hostname)}
{":authority", conn.authority}
| headers
]
else
[
{":method", method},
{":path", path},
{":scheme", conn.scheme},
{":authority", authority_pseudo_header(conn.scheme, conn.port, conn.hostname)}
{":authority", conn.authority}
| headers
]
end
end

@spec is_method?(proposed :: binary(), method :: charlist()) :: boolean()
defp is_method?(<<>>, []), do: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some benchmarking here to try to find the right replacement for the equality check:

Benchee script...
Mix.install([:benchee])
Code.ensure_loaded(String)

defmodule Helper do
  @spec is_method?(proposed :: binary(), method :: charlist()) :: boolean()
  def is_method?(<<>>, []), do: true

  def is_method?(<<char, rest_bin::binary>>, [char | rest_list]),
    do: is_method?(rest_bin, rest_list)

  def is_method?(<<lower_char, rest_bin::binary>>, [char | rest_list])
      when lower_char >= ?a and lower_char <= ?z and lower_char - 32 == char do
    is_method?(rest_bin, rest_list)
  end

  def is_method?(_proposed, _method), do: false

  @spec is_method_2?(proposed :: binary(), method :: binary()) :: boolean()
  def is_method_2?(method, method), do: true

  def is_method_2?(<<char, rest_proposed::binary>>, <<char, rest_method::binary>>),
    do: is_method_2?(rest_proposed, rest_method)

  def is_method_2?(<<lower_char, rest_proposed::binary>>, <<char, rest_method::binary>>)
      when lower_char >= ?a and lower_char <= ?z and lower_char - 32 == char do
    is_method_2?(rest_proposed, rest_method)
  end

  def is_method_2?(_proposed, _method), do: false
end

source = "CONNECT"

Benchee.run(
  %{
    "String.upcase/1" => fn ->
      String.upcase(source) == "CONNECT"
    end,
    "String.upcase(source, :ascii)" => fn ->
      String.upcase(source, :ascii) == "CONNECT"
    end,
    "Helper.is_method?" => fn ->
      Helper.is_method?(source, ~c"CONNECT")
    end,
    "Helper.is_method_2?" => fn ->
      Helper.is_method_2?(source, ~c"CONNECT")
    end
  },
  warmup: 3,
  time: 15,
  memory_time: 15
)
Results...
source = "POST"
---
Name                                    ips        average  deviation         median         99th %
Helper.is_method?                    6.99 M      143.04 ns ±40533.83%         100 ns         230 ns
Helper.is_method_2?                  4.85 M      206.05 ns ±34158.32%         130 ns         401 ns
String.upcase(source, :ascii)        3.76 M      265.78 ns ±21955.37%         190 ns         501 ns
String.upcase/1                      3.57 M      280.20 ns ±18536.86%         221 ns         541 ns

Comparison: 
Helper.is_method?                    6.99 M
Helper.is_method_2?                  4.85 M - 1.44x slower +63.00 ns
String.upcase(source, :ascii)        3.76 M - 1.86x slower +122.73 ns
String.upcase/1                      3.57 M - 1.96x slower +137.16 ns

Memory usage statistics:

Name                             Memory usage
Helper.is_method?                        40 B
Helper.is_method_2?                      64 B - 1.60x memory usage +24 B
String.upcase(source, :ascii)           128 B - 3.20x memory usage +88 B
String.upcase/1                         192 B - 4.80x memory usage +152 B



source = "connect"
---
Name                                    ips        average  deviation         median         99th %
Helper.is_method?                    6.18 M      161.80 ns ±34822.53%         130 ns         230 ns
Helper.is_method_2?                  4.79 M      208.91 ns ±33755.96%         161 ns         411 ns
String.upcase(source, :ascii)        3.20 M      312.49 ns ±21079.05%         221 ns         521 ns
String.upcase/1                      2.69 M      371.45 ns ±17205.79%         241 ns        1733 ns

Comparison: 
Helper.is_method?                    6.18 M
Helper.is_method_2?                  4.79 M - 1.29x slower +47.11 ns
String.upcase(source, :ascii)        3.20 M - 1.93x slower +150.69 ns
String.upcase/1                      2.69 M - 2.30x slower +209.66 ns

Memory usage statistics:

Name                             Memory usage
Helper.is_method?                        40 B
Helper.is_method_2?                      64 B - 1.60x memory usage +24 B
String.upcase(source, :ascii)           176 B - 4.40x memory usage +136 B
String.upcase/1                         288 B - 7.20x memory usage +248 B



source = "CONNECT"
---
Name                                    ips        average  deviation         median         99th %
Helper.is_method?                    6.62 M      150.99 ns ±19398.92%         120 ns         241 ns
Helper.is_method_2?                  4.95 M      202.14 ns ±25693.65%         160 ns         411 ns
String.upcase(source, :ascii)        3.26 M      307.22 ns ±22254.21%         220 ns         520 ns
String.upcase/1                      2.77 M      360.69 ns ±13072.92%         241 ns        1753 ns

Comparison: 
Helper.is_method?                    6.62 M
Helper.is_method_2?                  4.95 M - 1.34x slower +51.14 ns
String.upcase(source, :ascii)        3.26 M - 2.03x slower +156.22 ns
String.upcase/1                      2.77 M - 2.39x slower +209.69 ns

Memory usage statistics:

Name                             Memory usage
Helper.is_method?                        40 B
Helper.is_method_2?                      64 B - 1.60x memory usage +24 B
String.upcase(source, :ascii)           176 B - 4.40x memory usage +136 B
String.upcase/1                         288 B - 7.20x memory usage +248 B

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you bench if a regex case-insensitive check performs much worse than this? Basically check for method =~ ~r/\Aconnect\z/i. Usually regexes are pretty fast and it would avoid the 15 additional LoCs 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah it looks like the regex ends up being a fair bit slower than the String.upcase/2s 😬

Benchee script
Mix.install([:benchee])
Code.ensure_loaded(String)

defmodule Helper do
  @spec is_method?(proposed :: binary(), method :: charlist()) :: boolean()
  def is_method?(<<>>, []), do: true

  def is_method?(<<char, rest_bin::binary>>, [char | rest_list]),
    do: is_method?(rest_bin, rest_list)

  def is_method?(<<lower_char, rest_bin::binary>>, [char | rest_list])
      when lower_char >= ?a and lower_char <= ?z and lower_char - 32 == char do
    is_method?(rest_bin, rest_list)
  end

  def is_method?(_proposed, _method), do: false

  @spec is_method_2?(proposed :: binary(), method :: binary()) :: boolean()
  def is_method_2?(method, method), do: true

  def is_method_2?(<<char, rest_proposed::binary>>, <<char, rest_method::binary>>),
    do: is_method_2?(rest_proposed, rest_method)

  def is_method_2?(<<lower_char, rest_proposed::binary>>, <<char, rest_method::binary>>)
      when lower_char >= ?a and lower_char <= ?z and lower_char - 32 == char do
    is_method_2?(rest_proposed, rest_method)
  end

  def is_method_2?(_proposed, _method), do: false
end

source = "POST"

Benchee.run(
  %{
    "String.upcase/1" => fn ->
      String.upcase(source) == "CONNECT"
    end,
    "String.upcase(source, :ascii)" => fn ->
      String.upcase(source, :ascii) == "CONNECT"
    end,
    "Helper.is_method?" => fn ->
      Helper.is_method?(source, ~c"CONNECT")
    end,
    "Helper.is_method_2?" => fn ->
      Helper.is_method_2?(source, ~c"CONNECT")
    end,
    "Regex case-insensitive match" => fn ->
      source =~ ~r/\Aconnect\z/i
    end
  },
  warmup: 3,
  time: 15,
  memory_time: 15
)
Results
source = "POST"
---
Name                                    ips        average  deviation         median         99th %
Helper.is_method?                    7.00 M      142.93 ns ±39330.15%          91 ns         231 ns
Helper.is_method_2?                  4.98 M      200.84 ns ±34584.97%         110 ns         401 ns
String.upcase(source, :ascii)        3.90 M      256.67 ns ±21923.01%         181 ns         491 ns
String.upcase/1                      3.56 M      280.76 ns ±18300.86%         221 ns         501 ns
Regex case-insensitive match         1.17 M      852.35 ns  ±7533.60%         761 ns         962 ns

Comparison: 
Helper.is_method?                    7.00 M
Helper.is_method_2?                  4.98 M - 1.41x slower +57.91 ns
String.upcase(source, :ascii)        3.90 M - 1.80x slower +113.74 ns
String.upcase/1                      3.56 M - 1.96x slower +137.83 ns
Regex case-insensitive match         1.17 M - 5.96x slower +709.42 ns

Memory usage statistics:

Name                             Memory usage
Helper.is_method?                        40 B
Helper.is_method_2?                      64 B - 1.60x memory usage +24 B
String.upcase(source, :ascii)           128 B - 3.20x memory usage +88 B
String.upcase/1                         192 B - 4.80x memory usage +152 B
Regex case-insensitive match             56 B - 1.40x memory usage +16 B



source = "connect"
---
Name                                    ips        average  deviation         median         99th %
Helper.is_method?                    6.06 M      165.14 ns ±34145.79%         130 ns         251 ns
Helper.is_method_2?                  4.96 M      201.66 ns ±33980.79%         110 ns         401 ns
String.upcase(source, :ascii)        3.23 M      309.30 ns ±20909.48%         221 ns         511 ns
String.upcase/1                      2.69 M      372.38 ns ±17259.43%         241 ns        1723 ns
Regex case-insensitive match         1.12 M      888.92 ns  ±7217.10%         792 ns        1002 ns

Comparison: 
Helper.is_method?                    6.06 M
Helper.is_method_2?                  4.96 M - 1.22x slower +36.52 ns
String.upcase(source, :ascii)        3.23 M - 1.87x slower +144.16 ns
String.upcase/1                      2.69 M - 2.25x slower +207.24 ns
Regex case-insensitive match         1.12 M - 5.38x slower +723.78 ns

Memory usage statistics:

Name                             Memory usage
Helper.is_method?                        40 B
Helper.is_method_2?                      64 B - 1.60x memory usage +24 B
String.upcase(source, :ascii)           176 B - 4.40x memory usage +136 B
String.upcase/1                         288 B - 7.20x memory usage +248 B
Regex case-insensitive match             56 B - 1.40x memory usage +16 B



source = "CONNECT"
---
Name                                    ips        average  deviation         median         99th %
Helper.is_method?                    6.37 M      157.06 ns ±35458.04%         120 ns         250 ns
Helper.is_method_2?                  4.97 M      201.05 ns ±34113.80%         110 ns         401 ns
String.upcase(source, :ascii)        3.27 M      306.06 ns ±21259.05%         221 ns         511 ns
String.upcase/1                      2.73 M      366.85 ns ±17525.86%         240 ns        1733 ns
Regex case-insensitive match         1.13 M      887.71 ns  ±6217.62%         802 ns        1022 ns

Comparison: 
Helper.is_method?                    6.37 M
Helper.is_method_2?                  4.97 M - 1.28x slower +43.99 ns
String.upcase(source, :ascii)        3.27 M - 1.95x slower +149.01 ns
String.upcase/1                      2.73 M - 2.34x slower +209.79 ns
Regex case-insensitive match         1.13 M - 5.65x slower +730.66 ns

Memory usage statistics:

Name                             Memory usage
Helper.is_method?                        40 B
Helper.is_method_2?                      64 B - 1.60x memory usage +24 B
String.upcase(source, :ascii)           176 B - 4.40x memory usage +136 B
String.upcase/1                         288 B - 7.20x memory usage +248 B
Regex case-insensitive match             56 B - 1.40x memory usage +16 B


defp is_method?(<<char, rest_bin::binary>>, [char | rest_list]) do
is_method?(rest_bin, rest_list)
end

defp is_method?(<<lower_char, rest_bin::binary>>, [char | rest_list])
when lower_char >= ?a and lower_char <= ?z and lower_char - 32 == char do
is_method?(rest_bin, rest_list)
end

defp is_method?(_proposed, _method), do: false

defp sort_pseudo_headers_to_front(headers) do
Enum.sort_by(headers, fn {key, _value} ->
not String.starts_with?(key, ":")
Expand Down Expand Up @@ -1719,15 +1743,6 @@ defmodule Mint.HTTP2 do
end
end

# If the port is the default for the scheme, don't add it to the :authority pseudo-header
defp authority_pseudo_header(scheme, port, hostname) do
if URI.default_port(scheme) == port do
hostname
else
"#{hostname}:#{port}"
end
end

defp join_cookie_headers(headers) do
# If we have 0 or 1 Cookie headers, we just use the old list of headers.
case Enum.split_with(headers, fn {name, _value} -> Util.downcase_ascii(name) == "cookie" end) do
Expand Down
44 changes: 27 additions & 17 deletions test/mint/http2/conn_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ defmodule Mint.HTTP2Test do
@server_pdict_key {__MODULE__, :http2_test_server}

setup :start_server_async
setup :maybe_change_default_scheme_port
setup :start_connection
setup :maybe_set_transport_mock

Expand Down Expand Up @@ -841,29 +842,21 @@ defmodule Mint.HTTP2Test do
assert HTTP2.open?(conn)
end

@tag :with_overridden_default_port
test ":authority pseudo-header does not include port if it is the scheme's default",
%{conn: conn} do
default_https_port = URI.default_port("https")

try do
# Override default https port for this test
URI.default_port("https", conn.port)

{conn, _ref} = open_request(conn)
{conn, _ref} = open_request(conn)

assert_recv_frames [headers(hbf: hbf)]
assert_recv_frames [headers(hbf: hbf)]

assert {":authority", authority} =
hbf
|> server_decode_headers()
|> List.keyfind(":authority", 0)
assert {":authority", authority} =
hbf
|> server_decode_headers()
|> List.keyfind(":authority", 0)

assert authority == conn.hostname
assert authority == conn.hostname

assert HTTP2.open?(conn)
after
URI.default_port("https", default_https_port)
end
assert HTTP2.open?(conn)
end

test "when there's a request body, the content-length header is passed if not present",
Expand Down Expand Up @@ -2374,6 +2367,23 @@ defmodule Mint.HTTP2Test do
%{}
end

defp maybe_change_default_scheme_port(%{
server_port: server_port,
with_overridden_default_port: _
}) do
default_https_port = URI.default_port("https")

on_exit(fn -> URI.default_port("https", default_https_port) end)

:ok = URI.default_port("https", server_port)

%{}
end

defp maybe_change_default_scheme_port(_context) do
%{}
end

defp recv_next_frames(n) do
server = Process.get(@server_pdict_key)
TestServer.recv_next_frames(server, n)
Expand Down