Feature: `sconcat` and `stimes`. #580

kindaro · 2024-04-12T07:17:28Z

Resolve #288.

There are two commits here.

Commit № 1 adds two new benchmarks in the Pure section: one for sconcat and one for stimes.
Commit № 2 adds specialized implementation of sconcat and stimes to the instance of Semigroup for strict Text.

The benchmarks can be run like so:

first_commit_hash='180645e'
pattern='$2 == "Pure" && ($4 == "sconcat" || $4 == "stimes") && $5 != "LazyText"'
git checkout "$first_commit_hash" &&
    cabal run text-benchmarks -- --pattern "$pattern" --timeout 10s --csv benchmarks.csv
git switch feature-sconcat-stimes &&
    cabal run text-benchmarks -- --pattern "$pattern" --timeout 10s --baseline benchmarks.csv --fail-if-slower 110

This will take a few minutes. You should see better times almost everywhere — only the tiny.sconcat will show worse times.

This is the report as seen on my machine:

All
  Pure
    tiny
      sconcat
        Text: FAIL
          38.6 ns ± 2.3 ns, 148% more than baseline
          Use -p '(($2=="Pure"&&($4=="sconcat"||$4=="stimes"))&&$5!="LazyText")&&/tiny.sconcat.Text/' to rerun this test only.
      stimes
        Text: OK
          70.7 ns ± 2.9 ns, 83% less than baseline
    ascii-small
      sconcat
        Text: OK
          18.6 μs ± 148 ns, 99% less than baseline
      stimes
        Text: OK
          125  μs ±  10 μs, 59% less than baseline
    ascii
      sconcat
        Text: OK
          28.7 ms ± 2.4 ms
      stimes
        Text: OK
          63.3 ms ± 5.6 ms, 78% less than baseline
    english
      sconcat
        Text: OK
          1.07 ms ± 7.4 μs
      stimes
        Text: OK
          4.40 ms ± 373 μs, 69% less than baseline
    russian
      sconcat
        Text: OK
          2.45 μs ± 224 ns, 95% less than baseline
      stimes
        Text: OK
          15.9 μs ± 736 ns, 62% less than baseline
    japanese
      sconcat
        Text: OK
          4.17 μs ± 175 ns, 97% less than baseline
      stimes
        Text: OK
          15.7 μs ± 1.4 μs, 63% less than baseline

kindaro · 2024-04-12T07:52:07Z

Tada, all checks have passed!

Bodigrim

Thanks for writing benchmarks!

src/Data/Text.hs

Why? Li-yao Xia explains: > 1. Asking to "replicate a string n times" with negative n is nonsense. > There must be an error in the definition of n, so throwing an > exception lets users be aware of that error and fix it. > 2. The default definition of `stimes` already throws an exception for `n > <= 0`. People haven't complained about it. Extending the definition > for `n = 0` is reasonable for a monoid. > 3. There could still be a case made in favor of making `stimes` less > partial and more similar to `replicate` (I think "replicate a string > n times" is nonsense as a sentence in natural language, but I don't > have a strong argument that code must follow natural language). Until > someone makes a good case for extending `stimes`, throwing an > exception for negative arguments is forward-compatible: we can extend > the function later (it would only break code that catches the > exception, a fishy thing to do). If we made it total now and changed > our minds later, that would be a more breaking change. haskell#580 (comment)

kindaro · 2025-04-21T11:35:42Z

I resolved all conversations and fixed everything that was talked about. I also rebased on top of the current master.

There are some failing checks that I do not understand and have no idea how to handle.

Bodigrim

Thanks!

kindaro · 2025-04-21T16:59:33Z

@Bodigrim At your service! >λ=

What do we do with the failed checks? What are my next steps?

Bodigrim · 2025-04-21T17:39:43Z

Let's wait until @Lysxia reviews #631, then you'll be able to rebase and see how it goes.

Lysxia · 2025-04-22T11:39:10Z

I merged #631 so you can rebase to see if more of the CI tests pass.

The constructors of `Integer` and the module they are exported from all changed between GHC 8 and 9.

Why? Li-yao Xia explains: > 1. Asking to "replicate a string n times" with negative n is nonsense. > There must be an error in the definition of n, so throwing an > exception lets users be aware of that error and fix it. > 2. The default definition of `stimes` already throws an exception for `n > <= 0`. People haven't complained about it. Extending the definition > for `n = 0` is reasonable for a monoid. > 3. There could still be a case made in favor of making `stimes` less > partial and more similar to `replicate` (I think "replicate a string > n times" is nonsense as a sentence in natural language, but I don't > have a strong argument that code must follow natural language). Until > someone makes a good case for extending `stimes`, throwing an > exception for negative arguments is forward-compatible: we can extend > the function later (it would only break code that catches the > exception, a fishy thing to do). If we made it total now and changed > our minds later, that would be a more breaking change. haskell#580 (comment)

Haddock does not support comments on instance methods. haskell/haddock#123

kindaro · 2025-04-22T15:34:37Z

I am not sure what was expected, but I see less rather than more…

Lysxia

All good. I see the same speedup in the benchmarks. This is a great PR.

CI failure is unrelated.

Why? Li-yao Xia explains: > 1. Asking to "replicate a string n times" with negative n is nonsense. > There must be an error in the definition of n, so throwing an > exception lets users be aware of that error and fix it. > 2. The default definition of `stimes` already throws an exception for `n > <= 0`. People haven't complained about it. Extending the definition > for `n = 0` is reasonable for a monoid. > 3. There could still be a case made in favor of making `stimes` less > partial and more similar to `replicate` (I think "replicate a string > n times" is nonsense as a sentence in natural language, but I don't > have a strong argument that code must follow natural language). Until > someone makes a good case for extending `stimes`, throwing an > exception for negative arguments is forward-compatible: we can extend > the function later (it would only break code that catches the > exception, a fishy thing to do). If we made it total now and changed > our minds later, that would be a more breaking change. #580 (comment)

Bodigrim · 2025-04-22T23:41:34Z

Thanks @kindaro!

Bodigrim reviewed Apr 12, 2024

View reviewed changes

src/Data/Text.hs Outdated Show resolved Hide resolved

kindaro force-pushed the feature-sconcat-stimes branch 2 times, most recently from b653fe4 to b425505 Compare April 13, 2024 09:41

Bodigrim reviewed Apr 14, 2024

View reviewed changes

src/Data/Text.hs Outdated Show resolved Hide resolved

kindaro force-pushed the feature-sconcat-stimes branch from b425505 to c314ce2 Compare April 16, 2024 08:14

Lysxia approved these changes Apr 16, 2024

View reviewed changes

kindaro commented Apr 17, 2024

View reviewed changes

src/Data/Text.hs Outdated Show resolved Hide resolved

kindaro marked this pull request as draft April 17, 2024 05:37

kindaro force-pushed the feature-sconcat-stimes branch from 799fe7c to 904d5f6 Compare April 21, 2025 11:18

kindaro requested review from Bodigrim and Lysxia April 21, 2025 11:33

Bodigrim approved these changes Apr 21, 2025

View reviewed changes

kindaro marked this pull request as ready for review April 22, 2025 07:34

kindaro added 7 commits April 22, 2025 22:15

Add benchmarks for semigroup methods.

47bbaf1

Add specialized implementation of semigroup methods.

866e585

Check that stimes works right in corner cases.

f7e1089

Make sure stimes works right in corner cases.

e22e8d6

Be abstract of the implementation of Integer.

37f2221

The constructors of `Integer` and the module they are exported from all changed between GHC 8 and 9.

Move the crash warning to the instance haddock.

9f63f10

Haddock does not support comments on instance methods. haskell/haddock#123

kindaro force-pushed the feature-sconcat-stimes branch from 904d5f6 to 9f63f10 Compare April 22, 2025 15:17

Lysxia approved these changes Apr 22, 2025

View reviewed changes

Lysxia merged commit 1994b13 into haskell:master Apr 22, 2025
25 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature: `sconcat` and `stimes`. #580

Feature: `sconcat` and `stimes`. #580

Uh oh!

kindaro commented Apr 12, 2024

Uh oh!

kindaro commented Apr 12, 2024

Uh oh!

Bodigrim left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kindaro commented Apr 21, 2025 •

edited

Loading

Uh oh!

Bodigrim left a comment

Uh oh!

kindaro commented Apr 21, 2025 •

edited

Loading

Uh oh!

Bodigrim commented Apr 21, 2025

Uh oh!

Lysxia commented Apr 22, 2025

Uh oh!

kindaro commented Apr 22, 2025

Uh oh!

Lysxia left a comment

Uh oh!

Uh oh!

Bodigrim commented Apr 22, 2025

Uh oh!

Uh oh!

Feature: sconcat and stimes. #580

Feature: sconcat and stimes. #580

Uh oh!

Conversation

kindaro commented Apr 12, 2024

Uh oh!

kindaro commented Apr 12, 2024

Uh oh!

Bodigrim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kindaro commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bodigrim left a comment

Choose a reason for hiding this comment

Uh oh!

kindaro commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bodigrim commented Apr 21, 2025

Uh oh!

Lysxia commented Apr 22, 2025

Uh oh!

kindaro commented Apr 22, 2025

Uh oh!

Lysxia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bodigrim commented Apr 22, 2025

Uh oh!

Uh oh!

Feature: `sconcat` and `stimes`. #580

Feature: `sconcat` and `stimes`. #580

kindaro commented Apr 21, 2025 •

edited

Loading

kindaro commented Apr 21, 2025 •

edited

Loading