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

Mint's downcase_ascii/1 is slower than String.downcase(str, :ascii) #420

Closed
IceDragon200 opened this issue Jan 16, 2024 · 5 comments · Fixed by #424
Closed

Mint's downcase_ascii/1 is slower than String.downcase(str, :ascii) #420

IceDragon200 opened this issue Jan 16, 2024 · 5 comments · Fixed by #424

Comments

@IceDragon200
Copy link
Contributor

Despite touting:

# Lowercases an ASCII string more efficiently than
# String.downcase/1.

The only thing it beats good old String.downcase(&1, :ascii) on is memory usage:

Operating System: Linux
CPU Information: Intel(R) Core(TM) i7-2710QE CPU @ 2.10GHz
Number of Available Cores: 8
Available memory: 15.54 GB
Elixir 1.16.0
Erlang 26.2.1
JIT enabled: true

Benchmark suite executing with the following configuration:
warmup: 3 s
time: 15 s
memory time: 15 s
reduction time: 0 ns
parallel: 1
inputs: none specified
Estimated total run time: 1 min 39 s

Benchmarking Mint.Core.Util.downcase_ascii ...
Benchmarking String.downcase :ascii ...
Benchmarking String.downcase :default ...
Calculating statistics...
Formatting results...

Name                                    ips        average  deviation         median         99th %
String.downcase :ascii              90.09 K       11.10 μs   ±304.29%       10.26 μs       20.66 μs
String.downcase :default            76.86 K       13.01 μs   ±185.62%       11.85 μs       31.74 μs
Mint.Core.Util.downcase_ascii       66.29 K       15.09 μs    ±19.64%       14.57 μs       24.94 μs

Comparison:
String.downcase :ascii              90.09 K
String.downcase :default            76.86 K - 1.17x slower +1.91 μs
Mint.Core.Util.downcase_ascii       66.29 K - 1.36x slower +3.99 μs

Memory usage statistics:

Name                             Memory usage
String.downcase :ascii                6.80 KB
String.downcase :default              8.01 KB - 1.18x memory usage +1.20 KB
Mint.Core.Util.downcase_ascii        0.125 KB - 0.02x memory usage -6.67969 KB

**All measurements for memory usage were the same**
#!/usr/bin/env -S mix run
Code.ensure_loaded(String)
Code.ensure_loaded(Mint.Core.Util)

source = String.duplicate("THE QUICK brown fox jumps over THE LAZY DOG", 10)
Benchee.run(
  %{
    "String.downcase :default" => fn ->
      String.downcase(source)
    end,

    "String.downcase :ascii" => fn ->
      String.downcase(source, :ascii)
    end,

    "Mint.Core.Util.downcase_ascii" => fn ->
      Mint.Core.Util.downcase_ascii(source)
    end,
  },
  warmup: 3,
  time: 15,
  memory_time: 15
)

I only bring this up as I investigate some poor http2 performance (the same request completes in 1ms for http1, but takes 43ms for http2, even when reusing connections), unsure if it's mint or cowboy at this point (hard to find a good http2 client/server to compare against locally)

But I found that and it kind irked me.

My only question: what is mint's "efficiency" goal here, is it performance or memory usage?

  • If its performance, then, there are the numbers, it's not doing well at all there
  • If it's memory usage, then it's very good at what it does
@whatyouhide
Copy link
Contributor

@IceDragon200 thanks for the issue and the detailed report! 💟

Interesting. One thing from your benchmarks is that the memory usage difference is wild, with Mint basically using no memory compared to String.downcase/2. The performance is indeed slightly slower, yeah.

It boils down to how these are implemented: Mint does a for binary comprehension, while Elixir constructs an improper list of bytes and then uses IO.iodata_to_binary/1 to turn it into a string.

Out of curiosity, could you please benchmark this implementation?

  def downcase_ascii(<<char, rest::binary>>) do
    <<downcase_ascii_char(char), downcase_ascii(rest)>>
  end

  def downcase_ascii(<<>>) do
    <<>>
  end

  def downcase_ascii_char(char) when char in ?A..?Z, do: char + 32
  def downcase_ascii_char(char), do: char

@whatyouhide whatyouhide changed the title Mint.Core.Util.downcase_ascii/1 is slower than String.downcase(&1, :ascii) Mint's downcase_ascii/1 is slower than String.downcase(str, :ascii) Jan 25, 2024
@IceDragon200
Copy link
Contributor Author

Had to adjust your code a tad bit, otherwise it wouldn't run:

  def downcase_ascii(<<char, rest::binary>>) do
    <<downcase_ascii_char(char), downcase_ascii(rest)::binary>>
  end

  def downcase_ascii(<<>>) do
    <<>>
  end

  def downcase_ascii_char(char) when char in ?A..?Z, do: char + 32
  def downcase_ascii_char(char), do: char

The error for future reference without ::binary:

** (ArgumentError) construction of binary failed: segment 2 of type 'integer': expected an integer but got: ""
    (mint 1.5.2) lib/mint/core/util.ex:135: Mint.Core.Util.downcase_ascii2/1
    (mint 1.5.2) lib/mint/core/util.ex:135: Mint.Core.Util.downcase_ascii2/1
    (benchee 1.3.0) lib/benchee/benchmark/collect/time.ex:17: Benchee.Benchmark.Collect.Time.collect/1
    (benchee 1.3.0) lib/benchee/benchmark/runner.ex:291: Benchee.Benchmark.Runner.collect/3
    (benchee 1.3.0) lib/benchee/benchmark/repeated_measurement.ex:91: Benchee.Benchmark.RepeatedMeasurement.do_determine_n_times/5
    (benchee 1.3.0) lib/benchee/benchmark/runner.ex:226: Benchee.Benchmark.Runner.measure_runtimes/4
    (benchee 1.3.0) lib/benchee/benchmark/runner.ex:102: Benchee.Benchmark.Runner.measure_scenario/2
    (elixir 1.16.0) lib/task/supervised.ex:101: Task.Supervised.invoke_mfa/2
Function: #Function<2.121299024/0 in Benchee.Utility.Parallel.map/2>
    Args: []

And the results (with patch):

Operating System: Linux
CPU Information: Intel(R) Core(TM) i7-2710QE CPU @ 2.10GHz
Number of Available Cores: 8
Available memory: 15.54 GB
Elixir 1.16.0
Erlang 26.2.1
JIT enabled: true

Benchmark suite executing with the following configuration:
warmup: 3 s
time: 15 s
memory time: 15 s
reduction time: 0 ns
parallel: 1
inputs: none specified
Estimated total run time: 2 min 12 s

Benchmarking Mint.Core.Util.downcase_ascii ...
Benchmarking Mint.Core.Util.downcase_ascii2 ...
Benchmarking String.downcase :ascii ...
Benchmarking String.downcase :default ...
Calculating statistics...
Formatting results...

Name                                     ips        average  deviation         median         99th %
String.downcase :ascii               88.63 K       11.28 μs   ±418.04%       10.21 μs       25.42 μs
String.downcase :default             72.96 K       13.71 μs   ±290.62%       11.85 μs       33.28 μs
Mint.Core.Util.downcase_ascii        63.16 K       15.83 μs    ±27.06%       14.86 μs       35.95 μs
Mint.Core.Util.downcase_ascii2        6.51 K      153.54 μs    ±61.60%      109.72 μs      498.80 μs

Comparison:
String.downcase :ascii               88.63 K
String.downcase :default             72.96 K - 1.21x slower +2.42 μs
Mint.Core.Util.downcase_ascii        63.16 K - 1.40x slower +4.55 μs
Mint.Core.Util.downcase_ascii2        6.51 K - 13.61x slower +142.25 μs

Memory usage statistics:

Name                              Memory usage
String.downcase :ascii                 6.80 KB
String.downcase :default               8.01 KB - 1.18x memory usage +1.20 KB
Mint.Core.Util.downcase_ascii         0.125 KB - 0.02x memory usage -6.67969 KB
Mint.Core.Util.downcase_ascii2        20.45 KB - 3.00x memory usage +13.64 KB

**All measurements for memory usage were the same**

Conclusion: Please don't use that

@whatyouhide
Copy link
Contributor

LOL ok yep, that's crazy banana pants.

Ok, so it looks like we can optimize for speed, gaining 30-40% on single-digit µs times, or memory, with what looks like a huge reduction in memory usage. At the same time I’m skeptical of this reduction in memory, seems a bit too good to be true.

@ericmj thoughts?

@ericmj
Copy link
Member

ericmj commented Feb 10, 2024

If it's not faster then I am all for removing the code.

@IceDragon200 Thanks for benchmarking this. Would you like to send a PR that replaces the function with String.downcase?

@IceDragon200
Copy link
Contributor Author

@ericmj PR created, now pending review

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

Successfully merging a pull request may close this issue.

3 participants