From e524ae07c14167b54a67b9e3440f29a33dd96f18 Mon Sep 17 00:00:00 2001 From: Patrick Scheid Date: Tue, 10 Nov 2026 21:47:22 +0100 Subject: [PATCH] Simplify the options cache, the benchmark doc and the CI The options of each format are built once at start-up and shared. A call with AlsoAccept or AcceptDays builds new ones, so the documentation says to keep those in a variable. This replaces a table of 768 atomic pointers that made every combination of options free, with the bit set, the format numbers and the tests that served it. BENCHMARKS.md keeps what tests enforce and one table marked as a snapshot, and the README quotes no nanoseconds. The benchmarks are cut to the ones the doc relies on, and the test that pinned exact allocation counts of errors is gone. CI fuzzes in one job instead of a generated matrix. The coverage badge job and the Scorecard workflow are removed, and the README keeps the CI and Go Reference badges. The README and the documentation of Format now give the reasons for three choices: a Format is a name and an invalid one a programming error, exponents are rejected on purpose, and a sign inside an ISO 8601 duration is rejected as the format tag of encoding/json/v2 did. --- .github/workflows/ci.yml | 83 +-------- .github/workflows/scorecard.yml | 41 ----- BENCHMARKS.md | 250 ++++--------------------- README.md | 23 +-- bench_test.go | 312 ++++---------------------------- decoder.go | 3 +- errors_test.go | 2 + format.go | 44 +---- format_test.go | 20 -- json.go | 28 ++- justfile | 8 - shared.go | 68 ++++--- shared_test.go | 143 ++++----------- 13 files changed, 186 insertions(+), 839 deletions(-) delete mode 100644 .github/workflows/scorecard.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4f4ab06..e30f935 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -93,34 +93,13 @@ jobs: - uses: extractions/setup-just@53165ef7e734c5c07cb06b3c8e7b647c5aa16db3 # v4 - run: just test-32bit - # Asks the tests of both modules for their fuzz targets, so that the fuzz job cannot miss a new one. - fuzz-targets: - runs-on: ubuntu-latest - timeout-minutes: 5 - outputs: - matrix: ${{ steps.list.outputs.matrix }} - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - persist-credentials: false - - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 - with: - go-version-file: go.mod - cache: false - - uses: extractions/setup-just@53165ef7e734c5c07cb06b3c8e7b647c5aa16db3 # v4 - - id: list - run: | - matrix=$(just fuzz-matrix) - echo "matrix=$matrix" >> "$GITHUB_OUTPUT" - + # Every fuzz target of both modules, one after the other. The recipes ask the tests for their targets, so a new one + # cannot be missed. A target runs for 45 seconds on a push and for 10 minutes at night. fuzz: - needs: fuzz-targets - name: fuzz (${{ matrix.target }}, ${{ matrix.recipe }}) - strategy: - fail-fast: false - matrix: ${{ fromJSON(needs.fuzz-targets.outputs.matrix) }} runs-on: ubuntu-latest - timeout-minutes: 20 + timeout-minutes: 90 + env: + FUZZTIME: ${{ github.event_name == 'schedule' && '10m' || '45s' }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -135,23 +114,19 @@ jobs: - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: path: ~/.cache/go-build/fuzz - key: fuzz-${{ matrix.target }}-${{ github.run_id }} - restore-keys: fuzz-${{ matrix.target }}- - - run: just "$RECIPE" "$FUZZTIME" "$TARGET" - env: - RECIPE: ${{ matrix.recipe }} - TARGET: ${{ matrix.target }} - FUZZTIME: ${{ github.event_name == 'schedule' && '10m' || '45s' }} + key: fuzz-${{ github.run_id }} + restore-keys: fuzz- + - run: just fuzz "$FUZZTIME" + - run: just compat-fuzz "$FUZZTIME" - name: Upload the failing input if: failure() uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 with: - name: fuzz-${{ matrix.target }} + name: fuzz-failing-input path: | testdata/fuzz internal/compat/testdata/fuzz - # Too slow for every push: every change to the code runs the tests again. mutation: if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' runs-on: ubuntu-latest @@ -177,41 +152,3 @@ jobs: go-version-input: stable check-latest: true cache: false - - # Publishes the measured coverage for the README badge, also when it drops below 100%, as a shields.io endpoint file - # on the badges branch. - coverage-badge: - if: github.ref == 'refs/heads/main' && github.event_name != 'pull_request' - runs-on: ubuntu-latest - timeout-minutes: 10 - permissions: - contents: write # push the badges branch - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - persist-credentials: false - - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 - with: - go-version-file: go.mod - cache: false - - uses: extractions/setup-just@53165ef7e734c5c07cb06b3c8e7b647c5aa16db3 # v4 - - run: just coverage - - name: Publish the badge - env: - GH_TOKEN: ${{ github.token }} - run: | - total=$(go tool cover -func=coverage.out | awk '/^total:/ { print $3 }') - color=$(awk -v t="${total%\%}" 'BEGIN { - print (t >= 100 ? "brightgreen" : t >= 90 ? "green" : t >= 75 ? "yellow" : "red") - }') - mkdir "$RUNNER_TEMP/badges" - cd "$RUNNER_TEMP/badges" - printf '{"schemaVersion":1,"label":"coverage","message":"%s","color":"%s"}\n' "$total" "$color" \ - > coverage.json - # One fresh commit each time, force-pushed, so the branch never grows. - git init -q -b badges - git add coverage.json - git -c user.name="github-actions[bot]" -c user.email="41898282+github-actions[bot]@users.noreply.github.com" \ - commit -q -m "Coverage ${total} at ${GITHUB_SHA::7}" - git push -q -f "https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" badges - echo "### Coverage badge: ${total}" >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml deleted file mode 100644 index c32888f..0000000 --- a/.github/workflows/scorecard.yml +++ /dev/null @@ -1,41 +0,0 @@ -# OpenSSF Scorecard rates the repository's security practices, such as pinned actions, token permissions and branch -# protection. The results go to the Security tab and, through publish_results, to the badge in the README. -name: Scorecard - -on: - push: - branches: [ main ] - schedule: - - cron: "41 5 * * 1" - branch_protection_rule: - workflow_dispatch: - -permissions: - contents: read - -jobs: - analysis: - runs-on: ubuntu-latest - timeout-minutes: 10 - permissions: - contents: read - actions: read - security-events: write # upload the results to code scanning - id-token: write # publish the results to the badge - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - persist-credentials: false - - uses: ossf/scorecard-action@fa7117db83c52d787ae5a89f350999701505d78e # v2.5.0 - with: - results_file: results.sarif - results_format: sarif - publish_results: true - - uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 - with: - name: scorecard - path: results.sarif - retention-days: 5 - - uses: github/codeql-action/upload-sarif@33d7b85da1a04171e211befe2c739303b9a4c066 # v4.39.3 - with: - sarif_file: results.sarif diff --git a/BENCHMARKS.md b/BENCHMARKS.md index bc31a23..f9e4775 100644 --- a/BENCHMARKS.md +++ b/BENCHMARKS.md @@ -1,212 +1,42 @@ # Benchmarks -What a duration costs with jsonduration, next to what the standard library takes for the same value without it. - -- **It allocates nothing of its own.** Writing and reading a duration with `encoding/json/v2` allocates what an - `int64` in the same place does: the JSON that `json.Marshal` returns, and nothing on reading. Only an error - allocates. -- **`With` is cheap to call.** `With`, `Marshalers`, and `Unmarshalers` take 5 to 13 ns and allocate nothing, so there - is no need to keep what they return. -- **A duration in a struct field takes about 50 ns more to write and 60 to 100 ns more to read than an `int64`.** - About 35 ns of that is what the standard library takes to call any function for a type. The rest is the format. -- **Wrapper types are faster than options in a struct with many fields.** With options, the standard library asks for - every field whether a function applies to it. With wrapper types it does not. - -## How they were measured - -- With Go 1.27.3 on a MacBook Pro with an Apple M2 and 16 GB of memory, macOS 27.0.1, `darwin/arm64`, in November - 2026. -- Every number is the median of ten runs of half a second each. `just bench` runs every benchmark once instead. - - ```sh - go test -run '^$' -bench . -benchmem -benchtime 500ms -count 10 - ``` - -- Times are in nanoseconds for one operation. Two runs of the same benchmark differ by one or two percent, so a few - nanoseconds between two rows mean nothing. -- The duration is 1h30m0.5s unless a section says otherwise: `"1h30m0.5s"`, `5400.5`, `5400500`, `5400500000`, - `5400500000000`, and `"PT1H30M0.5S"` in the six formats. -- The benchmarks are in [bench_test.go](bench_test.go). Each heading below names its benchmark. - -The tables compare jsonduration with four ways to do without it: - -- **`int64`.** The field is an `int64` and there are no options. This is what a number costs in the standard library. -- **Nanosecond option.** A `time.Duration` with `jsonv1.FormatDurationAsNano(true)`, the standard library's own way to - write a duration, as nanoseconds only. -- **Smallest function.** A `time.Duration` with `json.WithMarshalers` and `json.WithUnmarshalers`, and functions that - do nothing but write and read the nanoseconds as an integer. `With` passes its functions the same way, so this is - the least that it could cost. -- **Text methods.** A type of your own with `MarshalText` and `UnmarshalText`, which call `time.Duration.String` and - `time.ParseDuration`. This is what one writes by hand. - -## Building the options - -`BenchmarkOptions` - -| Call | Time | Allocations | -|---------------------------------------------------------|-----:|------------:| -| `With(ISO8601)` | 5.5 | 0 | -| `With(ISO8601, AlsoAccept(Nano, Units), AcceptDays())` | 12.8 | 0 | -| `Marshalers(ISO8601)` | 5.1 | 0 | -| `Unmarshalers(ISO8601, AlsoAccept(Nano))` | 8.7 | 0 | - -The options are built once for each set of arguments and shared afterwards. Versions up to v0.3.1 built them on -every call, which took about 260 ns and 13 allocations for `With(ISO8601)`, so keep the options in a variable there. - -## Writing one duration - -`BenchmarkMarshal`: `json.Marshal` of a pointer to a `struct{ D time.Duration }`. - -| Format | Options kept | `With` on every call | Wrapper type | -|-----------|-------------:|---------------------:|-------------:| -| `Units` | 152 | 159 | 148 | -| `Sec` | 156 | 161 | 146 | -| `Milli` | 152 | 159 | 141 | -| `Micro` | 155 | 160 | 139 | -| `Nano` | 157 | 162 | 143 | -| `ISO8601` | 157 | 161 | 141 | - -Each of these allocates once, 16 or 24 bytes: the JSON that `json.Marshal` returns. - -| Without jsonduration | Time | Allocations | -|----------------------|-----:|------------:| -| `int64` | 102 | 1 | -| Nanosecond option | 104 | 1 | -| Smallest function | 136 | 1 | -| Text methods | 151 | 2 | - -`BenchmarkMarshalWrite` writes into an `io.Writer` with `json.MarshalWrite`, which returns no JSON. Then nothing is -left to allocate: - -| Way | Time | Allocations | -|--------------------------|-----------:|------------:| -| jsonduration, any format | 145 to 162 | 0 | -| `int64` | 94 | 0 | -| Nanosecond option | 97 | 0 | -| Smallest function | 131 | 0 | -| Text methods | 154 | 1 | - -## Reading one duration - -`BenchmarkUnmarshal`: `json.Unmarshal` into the same struct. - -| Format | Options kept | `With` on every call | Wrapper type | -|-----------|-------------:|---------------------:|-------------:| -| `Units` | 207 | 208 | 182 | -| `Sec` | 180 | 187 | 153 | -| `Milli` | 174 | 174 | 145 | -| `Micro` | 177 | 178 | 149 | -| `Nano` | 180 | 181 | 153 | -| `ISO8601` | 212 | 213 | 183 | - -None of these allocates. - -| Without jsonduration | Time | Allocations | -|----------------------|-----:|------------:| -| `int64` | 115 | 0 | -| Nanosecond option | 116 | 0 | -| Smallest function | 152 | 0 | -| Text methods | 174 | 1 | - -## Reading more than one format - -`BenchmarkUnmarshalAlsoAccept` - -| Options | Input | Time | Allocations | -|-------------------------------------|-------------------|-----:|------------:| -| `With(ISO8601)` | `"PT1H30M0.5S"` | 211 | 0 | -| `With(ISO8601, AlsoAccept(Units))` | `"PT1H30M0.5S"` | 212 | 0 | -| `With(ISO8601, AlsoAccept(Units))` | `"1h30m0.5s"` | 213 | 0 | -| `With(ISO8601, AlsoAccept(Nano))` | `5400500000000` | 181 | 0 | - -A second format costs nothing that can be measured, whether the input is in the first format or in the second. - -## Many durations - -`BenchmarkSlice`: a `[]time.Duration` of a thousand durations, from 1h30m0.5s on in steps of one second. The times -are for one duration, a thousandth of the whole. - -| Way | Writing | Reading | -|-------------------|--------:|--------:| -| `Units` | 46.6 | 101.0 | -| `Sec` | 47.6 | 65.1 | -| `Milli` | 41.6 | 59.1 | -| `Micro` | 43.4 | 64.2 | -| `Nano` | 45.1 | 68.7 | -| `ISO8601` | 47.0 | 106.4 | -| `int64` | 14.8 | 33.2 | -| Nanosecond option | 16.3 | 34.4 | -| Smallest function | 27.2 | 41.9 | - -Writing the thousand allocates once, for the JSON, and reading into a slice that is long enough allocates nothing, in -every row. The number of allocations does not grow with the number of durations. - -## A struct with other fields - -`BenchmarkConfig`: nine values, five of them durations. - -```go -type config struct { - Name string - Address string - Retries int - Verbose bool - Timeout time.Duration - Interval time.Duration - Backoff []time.Duration // three of them -} -``` - -| Way | Writing, options | Writing, wrapper types | Reading, options | Reading, wrapper types | -|-------------------|-----------------:|-----------------------:|-----------------:|-----------------------:| -| `Units` | 605 | 533 | 825 | 729 | -| `Sec` | 609 | 521 | 818 | 697 | -| `Milli` | 616 | 534 | 816 | 687 | -| `Micro` | 618 | 533 | 819 | 709 | -| `Nano` | 632 | 548 | 837 | 733 | -| `ISO8601` | 581 | 495 | 908 | 800 | -| `int64` | 346 | | 535 | | -| Nanosecond option | 350 | | 536 | | -| Smallest function | 527 | | 681 | | - -Writing allocates once, for the JSON, and reading allocates nothing, in every row. - -The smallest function takes 180 ns more than `int64` to write this struct, but only 35 ns more to write the struct -with one field. That is because the standard library, once it has functions from `json.WithMarshalers` or -`json.WithUnmarshalers`, asks for every field whether one of them applies to its type, each time it writes or reads -the struct. The strings, the integer, and the boolean pay for it too. A wrapper type needs no options, so nothing is -asked, and that is why wrapper types are 70 to 130 ns faster here. - -## Invalid input - -`BenchmarkUnmarshalInvalid`: `{"D":"1h30m"}`, which is rejected. The error builds its message when `Error` is -called, so the table has the cost for a caller who only checks the error, with `errors.Is` for example, and for one -who also reads the message. - -| Way | Checked: time | Bytes | Allocations | Read: time | Bytes | Allocations | -|-----------------------------------|--------------:|------:|------------:|-----------:|------:|------------:| -| `With(ISO8601)` | 292 | 192 | 5 | 516 | 528 | 13 | -| `int64`, which rejects the string | 169 | 128 | 3 | 273 | 256 | 8 | - -An error is the one thing that allocates. Until its message is read, a syntax error like this one takes two -allocations more than the standard library's own error: the input that it quotes, and the error itself. The message -says where the duration is, quotes the input, and names the accepted formats and the cause, which the standard -library's message for an `int64` does not. - -## Without JSON - -`BenchmarkFormat` and `BenchmarkParse`: `Format.Format` and `Format.Parse`, next to the two functions of package -`time`. - -| Format | `Format` | Bytes | Allocations | `Parse` | Allocations | -|----------------------------------------------|---------:|------:|------------:|--------:|------------:| -| `Units` | 29.9 | 16 | 1 | 59.9 | 0 | -| `Sec` | 19.6 | 8 | 1 | 20.5 | 0 | -| `Milli` | 19.4 | 8 | 1 | 20.8 | 0 | -| `Micro` | 21.6 | 16 | 1 | 22.1 | 0 | -| `Nano` | 22.8 | 16 | 1 | 24.1 | 0 | -| `ISO8601` | 31.5 | 16 | 1 | 58.7 | 0 | -| `time.Duration.String`, `time.ParseDuration` | 22.7 | 16 | 1 | 45.0 | 0 | - -`Format` allocates the string that it returns. `Parse` allocates nothing for input of up to 32 bytes and once for -longer input. Nothing that `Format` writes is longer than that. +## What the tests enforce + +These hold on every platform and Go version that CI runs, because a test fails otherwise: + +- **Writing and reading a duration allocates nothing of its own.** What is allocated is what the standard library + allocates for an `int64` in the same place: the JSON that `json.Marshal` returns, and nothing on reading. +- **`With`, `Marshalers`, and `Unmarshalers` allocate nothing without decoding options.** The options of each format + are built once, so there is no need to keep what these return. With `AlsoAccept` or `AcceptDays` they build new + options on every call, so keep those in a variable. Versions up to v0.3.1 built every set of options anew. +- **`Format.Format` allocates the string it returns, and `Format.Parse` nothing** for what `Format.Format` writes. + +Only an error allocates. It builds its message when `Error` is called, so a caller who only checks it with +`errors.Is` does not pay for the message. + +## Roughly how long it takes + +One duration of 1h30m0.5s in a `struct{ D time.Duration }`, in nanoseconds, with Go 1.27.3 on an Apple M2 in November +2026. This is one measurement on one machine and will drift, so take the proportions from it and not the numbers. + +| Way | Writing | Reading | +|----------------------------------------------------------------|-----------:|-----------:| +| An `int64` field, without options | 102 | 115 | +| A function that does nothing but write or read the nanoseconds | 136 | 152 | +| A type of your own with `MarshalText` and `UnmarshalText` | 151 | 174 | +| `With`, in the six formats | 152 to 157 | 174 to 212 | +| A wrapper type such as `ISO8601Duration`, in the six formats | 139 to 148 | 145 to 183 | + +- **A large part of the difference to an `int64` is the standard library's.** The second row passes its function the + way `With` does, with `json.WithMarshalers` and `json.WithUnmarshalers`, so it is the least that `With` could cost. +- **A type of your own is no faster.** It also allocates once more than the other rows, each way. +- **Wrapper types are faster than options in a struct with many fields.** Once it has functions, the standard library + asks for every field whether one of them applies to its type, so the fields that are not durations pay too. A + wrapper type needs no options. +- **A second format costs nothing that can be measured.** Reading with `AlsoAccept` takes as long as without, whether + the input is in the first format or in the second. + +## Measuring it yourself + +`just bench` runs the benchmarks in [bench_test.go](bench_test.go): writing and reading one duration, `AlsoAccept`, +a struct with other fields, and `Format` and `Parse`, each next to the ways above to do without the package. diff --git a/README.md b/README.md index 169d972..049c9fd 100644 --- a/README.md +++ b/README.md @@ -2,8 +2,6 @@ [![CI](https://github.com/pscheid92/jsonduration/actions/workflows/ci.yml/badge.svg)](https://github.com/pscheid92/jsonduration/actions/workflows/ci.yml) [![Go Reference](https://pkg.go.dev/badge/github.com/pscheid92/jsonduration.svg)](https://pkg.go.dev/github.com/pscheid92/jsonduration) -[![Coverage](https://img.shields.io/endpoint?url=https%3A%2F%2Fraw.githubusercontent.com%2Fpscheid92%2Fjsonduration%2Fbadges%2Fcoverage.json)](https://github.com/pscheid92/jsonduration/actions/workflows/ci.yml) -[![OpenSSF Scorecard](https://api.scorecard.dev/projects/github.com/pscheid92/jsonduration/badge)](https://scorecard.dev/viewer/?uri=github.com/pscheid92/jsonduration) JSON formats for `time.Duration` in Go's `encoding/json/v2`. @@ -55,8 +53,9 @@ func main() { } ``` -`encoding/json/v2` is part of the standard library since Go 1.27, with no `GOEXPERIMENT` setting. `With` builds the -options on the first call and returns the same ones afterwards, so a later call costs a lookup and no allocation. +`encoding/json/v2` is part of the standard library since Go 1.27, with no `GOEXPERIMENT` setting. Without decoding +options, `With` returns the same options on every call and allocates nothing. With `AlsoAccept` or `AcceptDays` it +builds new ones, so keep those in a variable. If you already pass `json.WithMarshalers` or `json.WithUnmarshalers`, don't add `With` next to them: a second `WithMarshalers` option replaces the first, without an error. Join yours with the package's instead: @@ -208,7 +207,9 @@ flag.Parse() opts := jsonduration.With(format) ``` -`With` panics on an invalid `Format`, which includes the empty one. Check a `Format` from a plain string first: +A `Format` is its name, which is what lets a flag or a configuration hold one. So a `Format` that names nothing can +exist, like `time.Month(13)`, and using it is a programming error: `With` panics on an invalid `Format`, which +includes the empty one. Check a `Format` from a plain string first: ```go format := jsonduration.Format(os.Getenv("DURATION_FORMAT")) @@ -233,12 +234,13 @@ d, err := jsonduration.Sec.Parse("5400.5") // 1h30m0.5s - To leave out zero durations, use `omitzero`. In v2, `omitempty` keeps them. - Decoding checks the format, not the value: `"-1s"` and `"0s"` are read. Check ranges after decoding. - Numeric formats reject exponents, as in `1e-05`, which some encoders such as Swift's write for small numbers. + That is on purpose: decoding is strict, and exponents would add a second spelling of every number for a rare case. - Digits below a nanosecond are truncated, not rounded. No format rounds to whole units: `Milli` writes `1.234567`. - The largest duration is about 292 years, `PT2562047H47M16.854775807S`; anything larger is `strconv.ErrRange`. - `ISO8601` takes one sign, `-` or `+`, only in front, as in `-PT30S`. It rejects Java's negative durations, such as - `PT-30S`. It reads designators in either case and a decimal comma, so `pt1,5s` is 1.5s. A count of zero must have - no fraction: `"P0D"` is read, `"P0.0D"` is not. Output is always the shortest form, so `"P0DT2H"` comes back as - `"PT2H"`. + `PT-30S`, as the `format` tag of `encoding/json/v2` did. It reads designators in either case and a decimal comma, + so `pt1,5s` is 1.5s. A count of zero must have no fraction: `"P0D"` is read, `"P0.0D"` is not. Output is always the + shortest form, so `"P0DT2H"` comes back as `"PT2H"`. - `Units` reads what `time.ParseDuration` reads. So it reads protobuf's `"3600s"` but writes `"1h0m0s"`, and it does not read Rust humantime's `"1h 30m"` or PostgreSQL's `"01:30:00"`. PostgreSQL writes ISO 8601 with `intervalstyle = iso_8601`. @@ -263,9 +265,8 @@ them. [TESTING.md](TESTING.md) has the details. ## How fast it is jsonduration allocates nothing of its own: writing and reading a duration allocates what an `int64` in the same place -does. A duration in a struct field takes about 50 ns more to write and 60 to 100 ns more to read than an `int64`, on -an Apple M2. `With` takes 5 ns, so there is no need to keep what it returns. In a struct with many fields, wrapper -types are faster than options. [BENCHMARKS.md](BENCHMARKS.md) has the numbers and what they are compared with. +does, and `With` without decoding options allocates nothing. Tests enforce both. In a struct with many fields, +wrapper types are faster than options. [BENCHMARKS.md](BENCHMARKS.md) has rough timings and the reasons. ## Development diff --git a/bench_test.go b/bench_test.go index 44f1674..d7668b6 100644 --- a/bench_test.go +++ b/bench_test.go @@ -1,10 +1,8 @@ package jsonduration import ( - jsonv1 "encoding/json" "encoding/json/jsontext" "encoding/json/v2" - "errors" "io" "testing" "time" @@ -53,43 +51,6 @@ func TestAllocations(t *testing.T) { } } -// An error allocates the input it quotes and itself. Its message is built when asked for, which this does not do. -func TestRejectedDurationAllocations(t *testing.T) { - var err error - - parse := func(f Format, in string) func() { - return func() { _, err = f.Parse(in) } - } - - // An unknown unit is an error of its own inside the other, with the name of the unit. - tests := []struct { - name string - want float64 - fn func() - }{ - {"ISO8601.Parse", 2, parse(ISO8601, "1h30m")}, - {"Units.Parse with an unknown unit", 4, parse(Units, "5fortnights")}, - } - - for _, tt := range tests { - if allocs := testing.AllocsPerRun(100, tt.fn); allocs != tt.want || err == nil { - t.Errorf("%s: %v allocs with error %v, want %v allocs and an error", tt.name, allocs, err, tt.want) - } - } - - // What encoding/json/v2 allocates around the error is its own business, and differs between platforms. - // So this measures the decoder alone, which builds the names of its formats once and not for every error. - for _, opts := range [][]DecodeOption{nil, {AlsoAccept(Nano, Units), AcceptDays()}} { - dc := newDecoder(ISO8601, configFrom(opts)) - in := jsontext.Value(`"PT1X"`) - - allocs := testing.AllocsPerRun(100, func() { _, err = dc.parse(in, false) }) - if allocs != 2 || !errors.Is(err, ErrInvalidDuration) { - t.Errorf("decoder in format %s: %v allocs with error %v, want 2 allocs and an error", dc.names, allocs, err) - } - } -} - // The benchmarks below are the ones BENCHMARKS.md reports. Their names are //, // with "baseline" in place of the format for what to compare with. @@ -113,11 +74,8 @@ func (d *textDuration) UnmarshalText(b []byte) error { return err } -// nanoOption is the option of the standard library that writes a time.Duration as nanoseconds. -var nanoOption = jsonv1.FormatDurationAsNano(true) - -// nanoFuncs does the same with functions, as With does, and does as little in them as a function can. What it costs -// more than nanoOption is what the standard library takes to call a function for a type. +// nanoFuncs passes functions as With does, and does as little in them as a function can: it writes and reads +// nanoseconds. What it costs more than an int64 is what the standard library takes to call a function for a type. var nanoFuncs = json.JoinOptions( json.WithMarshalers(json.MarshalToFunc(func(enc *jsontext.Encoder, d time.Duration) error { return enc.WriteToken(jsontext.Int(int64(d))) @@ -159,48 +117,6 @@ func configOf[T any](wrap func(time.Duration) T) config[T] { } } -// benchWrapperConfig measures a config whose durations have the wrapper type of format f: decoding in, or encoding -// such a config if in is nil. -func benchWrapperConfig(b *testing.B, f Format, in []byte) { - b.Helper() - - switch f { - case Units: - benchConfigOf(b, func(d time.Duration) UnitsDuration { return UnitsDuration{d} }, in) - case Sec: - benchConfigOf(b, func(d time.Duration) SecDuration { return SecDuration{d} }, in) - case Milli: - benchConfigOf(b, func(d time.Duration) MilliDuration { return MilliDuration{d} }, in) - case Micro: - benchConfigOf(b, func(d time.Duration) MicroDuration { return MicroDuration{d} }, in) - case Nano: - benchConfigOf(b, func(d time.Duration) NanoDuration { return NanoDuration{d} }, in) - case ISO8601: - benchConfigOf(b, func(d time.Duration) ISO8601Duration { return ISO8601Duration{d} }, in) - } -} - -func benchConfigOf[T any](b *testing.B, wrap func(time.Duration) T, in []byte) { - b.Helper() - - if in == nil { - benchMarshal(b, configOf(wrap)) - return - } - - benchUnmarshal[config[T]](b, in) -} - -// manyDurations returns n durations of different lengths, about as long as d90. -func manyDurations(n int) []time.Duration { - durations := make([]time.Duration, n) - for i := range durations { - durations[i] = d90 + time.Duration(i)*time.Second - } - - return durations -} - // benchMarshal measures json.Marshal of a pointer to v, so that handing v over does not allocate. func benchMarshal[T any](b *testing.B, v T, opts ...json.Options) { b.Helper() @@ -213,18 +129,6 @@ func benchMarshal[T any](b *testing.B, v T, opts ...json.Options) { } } -// benchMarshalWrite measures json.MarshalWrite, which returns no JSON and so does not allocate for it. -func benchMarshalWrite[T any](b *testing.B, v T, opts ...json.Options) { - b.Helper() - b.ReportAllocs() - - for b.Loop() { - if err := json.MarshalWrite(io.Discard, &v, opts...); err != nil { - b.Fatal(err) - } - } -} - // benchUnmarshal measures json.Unmarshal of in into a T, which is reused between calls. func benchUnmarshal[T any](b *testing.B, in []byte, opts ...json.Options) { b.Helper() @@ -250,80 +154,54 @@ func mustMarshal(b *testing.B, v any, opts ...json.Options) []byte { return out } -// benchWrapper measures a field of the wrapper type of format f: decoding in, or encoding such a field if in is nil. -func benchWrapper(b *testing.B, f Format, in []byte) { +// benchValue measures decoding in into a T, or encoding v if in is nil. +func benchValue[T any](b *testing.B, v T, in []byte, opts ...json.Options) { + b.Helper() + + if in == nil { + benchMarshal(b, v, opts...) + return + } + + benchUnmarshal[T](b, in, opts...) +} + +// benchWrapper measures a value whose durations have the wrapper type of format f: a field, or a whole config. +// It decodes in, or encodes the value if in is nil. +func benchWrapper(b *testing.B, f Format, whole bool, in []byte) { b.Helper() switch f { case Units: - benchField(b, UnitsDuration{d90}, in) + benchWrapped(b, func(d time.Duration) UnitsDuration { return UnitsDuration{d} }, whole, in) case Sec: - benchField(b, SecDuration{d90}, in) + benchWrapped(b, func(d time.Duration) SecDuration { return SecDuration{d} }, whole, in) case Milli: - benchField(b, MilliDuration{d90}, in) + benchWrapped(b, func(d time.Duration) MilliDuration { return MilliDuration{d} }, whole, in) case Micro: - benchField(b, MicroDuration{d90}, in) + benchWrapped(b, func(d time.Duration) MicroDuration { return MicroDuration{d} }, whole, in) case Nano: - benchField(b, NanoDuration{d90}, in) + benchWrapped(b, func(d time.Duration) NanoDuration { return NanoDuration{d} }, whole, in) case ISO8601: - benchField(b, ISO8601Duration{d90}, in) + benchWrapped(b, func(d time.Duration) ISO8601Duration { return ISO8601Duration{d} }, whole, in) } } -// benchField measures decoding in into a field of the type of d, or encoding such a field if in is nil. -func benchField[T any](b *testing.B, d T, in []byte) { +func benchWrapped[T any](b *testing.B, wrap func(time.Duration) T, whole bool, in []byte) { b.Helper() - if in == nil { - benchMarshal(b, field[T]{d}) + if whole { + benchValue(b, configOf(wrap), in) return } - benchUnmarshal[field[T]](b, in) -} - -// options keeps results of With alive, so that the calls are not optimized away. -var ( - options json.Options - marshalers *json.Marshalers - unmarshalers *json.Unmarshalers -) - -func BenchmarkOptions(b *testing.B) { - b.Run("With", func(b *testing.B) { - b.ReportAllocs() - for b.Loop() { - options = With(ISO8601) - } - }) - - b.Run("With_AlsoAccept_AcceptDays", func(b *testing.B) { - b.ReportAllocs() - for b.Loop() { - options = With(ISO8601, AlsoAccept(Nano, Units), AcceptDays()) - } - }) - - b.Run("Marshalers", func(b *testing.B) { - b.ReportAllocs() - for b.Loop() { - marshalers = Marshalers(ISO8601) - } - }) - - b.Run("Unmarshalers", func(b *testing.B) { - b.ReportAllocs() - for b.Loop() { - unmarshalers = Unmarshalers(ISO8601, AlsoAccept(Nano)) - } - }) + benchValue(b, field[T]{wrap(d90)}, in) } func BenchmarkMarshal(b *testing.B) { v := field[time.Duration]{d90} b.Run("baseline/int64", func(b *testing.B) { benchMarshal(b, field[int64]{int64(d90)}) }) - b.Run("baseline/nano_option", func(b *testing.B) { benchMarshal(b, v, nanoOption) }) b.Run("baseline/nano_funcs", func(b *testing.B) { benchMarshal(b, v, nanoFuncs) }) b.Run("baseline/text_methods", func(b *testing.B) { benchMarshal(b, field[textDuration]{textDuration(d90)}) }) @@ -331,39 +209,15 @@ func BenchmarkMarshal(b *testing.B) { opts := With(f) b.Run(string(f)+"/options", func(b *testing.B) { benchMarshal(b, v, opts) }) - b.Run(string(f)+"/wrapper", func(b *testing.B) { benchWrapper(b, f, nil) }) - - b.Run(string(f)+"/With_per_call", func(b *testing.B) { - b.ReportAllocs() - for b.Loop() { - if _, err := json.Marshal(&v, With(f)); err != nil { - b.Fatal(err) - } - } - }) - } -} - -func BenchmarkMarshalWrite(b *testing.B) { - v := field[time.Duration]{d90} - - b.Run("baseline/int64", func(b *testing.B) { benchMarshalWrite(b, field[int64]{int64(d90)}) }) - b.Run("baseline/nano_option", func(b *testing.B) { benchMarshalWrite(b, v, nanoOption) }) - b.Run("baseline/nano_funcs", func(b *testing.B) { benchMarshalWrite(b, v, nanoFuncs) }) - b.Run("baseline/text_methods", func(b *testing.B) { benchMarshalWrite(b, field[textDuration]{textDuration(d90)}) }) - - for _, f := range Formats() { - opts := With(f) - b.Run(string(f)+"/options", func(b *testing.B) { benchMarshalWrite(b, v, opts) }) + b.Run(string(f)+"/wrapper", func(b *testing.B) { benchWrapper(b, f, false, nil) }) } } func BenchmarkUnmarshal(b *testing.B) { v := field[time.Duration]{d90} - nanos := mustMarshal(b, v, nanoOption) + nanos := mustMarshal(b, field[int64]{int64(d90)}) b.Run("baseline/int64", func(b *testing.B) { benchUnmarshal[field[int64]](b, nanos) }) - b.Run("baseline/nano_option", func(b *testing.B) { benchUnmarshal[field[time.Duration]](b, nanos, nanoOption) }) b.Run("baseline/nano_funcs", func(b *testing.B) { benchUnmarshal[field[time.Duration]](b, nanos, nanoFuncs) }) b.Run("baseline/text_methods", func(b *testing.B) { benchUnmarshal[field[textDuration]](b, mustMarshal(b, v, With(Units))) @@ -374,25 +228,15 @@ func BenchmarkUnmarshal(b *testing.B) { in := mustMarshal(b, v, opts) b.Run(string(f)+"/options", func(b *testing.B) { benchUnmarshal[field[time.Duration]](b, in, opts) }) - b.Run(string(f)+"/wrapper", func(b *testing.B) { benchWrapper(b, f, in) }) - - b.Run(string(f)+"/With_per_call", func(b *testing.B) { - b.ReportAllocs() - - var got field[time.Duration] - for b.Loop() { - if err := json.Unmarshal(in, &got, With(f)); err != nil { - b.Fatal(err) - } - } - }) + b.Run(string(f)+"/wrapper", func(b *testing.B) { benchWrapper(b, f, false, in) }) } } // BenchmarkUnmarshalAlsoAccept measures what a second accepted format costs. func BenchmarkUnmarshalAlsoAccept(b *testing.B) { v := field[time.Duration]{d90} - iso, units, nanos := mustMarshal(b, v, With(ISO8601)), mustMarshal(b, v, With(Units)), mustMarshal(b, v, nanoOption) + iso, units := mustMarshal(b, v, With(ISO8601)), mustMarshal(b, v, With(Units)) + nanos := mustMarshal(b, field[int64]{int64(d90)}) isoOnly := With(ISO8601) isoOrUnits := With(ISO8601, AlsoAccept(Units)) @@ -409,82 +253,16 @@ func BenchmarkUnmarshalAlsoAccept(b *testing.B) { b.Run("second_is_a_number", func(b *testing.B) { benchUnmarshal[field[time.Duration]](b, nanos, isoOrNano) }) } -// benchRejected measures json.Unmarshal of in into a T, which must fail, and with message also reading the error. -func benchRejected[T any](b *testing.B, in []byte, message bool, opts ...json.Options) { - b.Helper() - b.ReportAllocs() - - var got T - for b.Loop() { - err := json.Unmarshal(in, &got, opts...) - if err == nil { - b.Fatal("no error") - } - - if message { - formatted = err.Error() - } - } -} - -// BenchmarkUnmarshalInvalid measures a duration that is rejected. The error builds its message when asked, -// so there is one benchmark for a caller who only checks the error and one for a caller who reads it. -func BenchmarkUnmarshalInvalid(b *testing.B) { - in := []byte(`{"D":"1h30m"}`) - opts := With(ISO8601) - - b.Run("options/checked", func(b *testing.B) { benchRejected[field[time.Duration]](b, in, false, opts) }) - b.Run("options/read", func(b *testing.B) { benchRejected[field[time.Duration]](b, in, true, opts) }) - b.Run("baseline/int64/checked", func(b *testing.B) { benchRejected[field[int64]](b, in, false) }) - b.Run("baseline/int64/read", func(b *testing.B) { benchRejected[field[int64]](b, in, true) }) -} - -// BenchmarkSlice measures a thousand durations in one value, to show the cost of each and that allocations do not -// grow with their number. -func BenchmarkSlice(b *testing.B) { - const n = 1000 - - durations := manyDurations(n) - ints := make([]int64, n) - for i, d := range durations { - ints[i] = int64(d) - } - - nanos := mustMarshal(b, ints) - - b.Run("Marshal/baseline/int64", func(b *testing.B) { benchMarshal(b, ints) }) - b.Run("Marshal/baseline/nano_option", func(b *testing.B) { benchMarshal(b, durations, nanoOption) }) - b.Run("Marshal/baseline/nano_funcs", func(b *testing.B) { benchMarshal(b, durations, nanoFuncs) }) - b.Run("Unmarshal/baseline/int64", func(b *testing.B) { benchUnmarshal[[]int64](b, nanos) }) - b.Run("Unmarshal/baseline/nano_option", func(b *testing.B) { - benchUnmarshal[[]time.Duration](b, nanos, nanoOption) - }) - b.Run("Unmarshal/baseline/nano_funcs", func(b *testing.B) { - benchUnmarshal[[]time.Duration](b, nanos, nanoFuncs) - }) - - for _, f := range Formats() { - opts := With(f) - in := mustMarshal(b, durations, opts) - - b.Run("Marshal/"+string(f)+"/options", func(b *testing.B) { benchMarshal(b, durations, opts) }) - b.Run("Unmarshal/"+string(f)+"/options", func(b *testing.B) { benchUnmarshal[[]time.Duration](b, in, opts) }) - } -} - -// BenchmarkConfig measures a value with other fields than durations, to show what share the durations have. +// BenchmarkConfig measures a value with other fields than durations. With options, the standard library asks for +// every field whether a function applies to it, which a wrapper type avoids. func BenchmarkConfig(b *testing.B) { durations := configOf(func(d time.Duration) time.Duration { return d }) ints := configOf(func(d time.Duration) int64 { return int64(d) }) nanos := mustMarshal(b, ints) b.Run("Marshal/baseline/int64", func(b *testing.B) { benchMarshal(b, ints) }) - b.Run("Marshal/baseline/nano_option", func(b *testing.B) { benchMarshal(b, durations, nanoOption) }) b.Run("Marshal/baseline/nano_funcs", func(b *testing.B) { benchMarshal(b, durations, nanoFuncs) }) b.Run("Unmarshal/baseline/int64", func(b *testing.B) { benchUnmarshal[config[int64]](b, nanos) }) - b.Run("Unmarshal/baseline/nano_option", func(b *testing.B) { - benchUnmarshal[config[time.Duration]](b, nanos, nanoOption) - }) b.Run("Unmarshal/baseline/nano_funcs", func(b *testing.B) { benchUnmarshal[config[time.Duration]](b, nanos, nanoFuncs) }) @@ -494,22 +272,15 @@ func BenchmarkConfig(b *testing.B) { in := mustMarshal(b, durations, opts) b.Run("Marshal/"+string(f)+"/options", func(b *testing.B) { benchMarshal(b, durations, opts) }) - b.Run("Marshal/"+string(f)+"/wrapper", func(b *testing.B) { benchWrapperConfig(b, f, nil) }) + b.Run("Marshal/"+string(f)+"/wrapper", func(b *testing.B) { benchWrapper(b, f, true, nil) }) b.Run("Unmarshal/"+string(f)+"/options", func(b *testing.B) { benchUnmarshal[config[time.Duration]](b, in, opts) }) - b.Run("Unmarshal/"+string(f)+"/wrapper", func(b *testing.B) { benchWrapperConfig(b, f, in) }) + b.Run("Unmarshal/"+string(f)+"/wrapper", func(b *testing.B) { benchWrapper(b, f, true, in) }) } } func BenchmarkFormat(b *testing.B) { - b.Run("baseline/Duration.String", func(b *testing.B) { - b.ReportAllocs() - for b.Loop() { - formatted = d90.String() - } - }) - for _, f := range Formats() { b.Run(string(f), func(b *testing.B) { b.ReportAllocs() @@ -521,17 +292,6 @@ func BenchmarkFormat(b *testing.B) { } func BenchmarkParse(b *testing.B) { - b.Run("baseline/time.ParseDuration", func(b *testing.B) { - s := d90.String() - - b.ReportAllocs() - for b.Loop() { - if _, err := time.ParseDuration(s); err != nil { - b.Fatal(err) - } - } - }) - for _, f := range Formats() { s := f.Format(d90) diff --git a/decoder.go b/decoder.go index a28db9e..c238679 100644 --- a/decoder.go +++ b/decoder.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "reflect" + "slices" "strconv" "time" ) @@ -31,7 +32,7 @@ type decoder struct { func newDecoder(f Format, config decodeConfig) *decoder { formats := []Format{f} for _, g := range Formats() { - if g != f && config.accepted.has(g) { + if g != f && slices.Contains(config.accepted, g) { formats = append(formats, g) } } diff --git a/errors_test.go b/errors_test.go index 5d12bd0..7e71a9a 100644 --- a/errors_test.go +++ b/errors_test.go @@ -33,6 +33,8 @@ func TestInvalidFormatPanics(t *testing.T) { want string }{ {"With", func() { With(bogus) }, "invalid format"}, + {"With the empty format", func() { With("") }, `invalid format ""`}, + {"With an invalid format after a valid option", func() { With(bogus, AlsoAccept(Nano)) }, "invalid format"}, {"Marshalers", func() { Marshalers(bogus) }, "invalid format"}, {"Unmarshalers", func() { Unmarshalers(bogus) }, "invalid format"}, {"Format", func() { bogus.Format(time.Second) }, "invalid format"}, diff --git a/format.go b/format.go index da9c564..6a0f509 100644 --- a/format.go +++ b/format.go @@ -5,7 +5,8 @@ import "time" // Format is a way to write a [time.Duration] in JSON, such as [ISO8601] for "PT1H30M". // Use it with [With], or convert single durations with [Format.Format] and [Format.Parse]. // -// A flag ([flag.TextVar]) or a JSON configuration can name a format, and both reject unknown names. +// A Format is its name, so that a flag ([flag.TextVar]) or a JSON configuration can hold one, and both reject unknown +// names. As with time.Month(13), a value that names no format can therefore exist, and using it is a programming error: // [With] panics on an invalid Format, such as the empty one that decoding leaves for a missing name or null, // so give it a default, as in cmp.Or(cfg.DurationFormat, jsonduration.ISO8601). // A Format converted from a string, as in Format(os.Getenv("DURATION_FORMAT")), is not checked, @@ -40,9 +41,6 @@ const ( ISO8601 Format = "iso8601" ) -// numFormats is the number of formats. TestFormatNumbers checks it. -const numFormats = 6 - // Formats returns all formats, for example to list them in a flag's usage. func Formats() []Format { return []Format{Units, Sec, Milli, Micro, Nano, ISO8601} @@ -180,44 +178,6 @@ func (f Format) codec() (codec, bool) { return codec{}, false } -// number returns the place of f in [Formats], counted from one, and zero if f is not valid. -func (f Format) number() uint8 { - switch f { - case Units: - return 1 - case Sec: - return 2 - case Milli: - return 3 - case Micro: - return 4 - case Nano: - return 5 - case ISO8601: - return 6 - } - - return 0 -} - -// A formatSet holds formats as bits, so that a set fits into one byte and has one value whatever order it was built in. -type formatSet uint8 - -// bit returns the set of f alone, and the empty set if f is not valid. -func (f Format) bit() formatSet { - number := f.number() - if number == 0 { - return 0 - } - - return 1 << (number - 1) -} - -// has reports whether f is in the set. -func (s formatSet) has(f Format) bool { - return s&f.bit() != 0 -} - // mustCodec returns the codec of format f, and panics if f is not valid. func (f Format) mustCodec() codec { c, ok := f.codec() diff --git a/format_test.go b/format_test.go index cbaab77..5b9ecc2 100644 --- a/format_test.go +++ b/format_test.go @@ -263,23 +263,3 @@ func FuzzParse(f *testing.F) { } }) } - -// TestFormatNumbers checks that number counts the formats in the order of Formats, and that numFormats is their count. -func TestFormatNumbers(t *testing.T) { - formats := Formats() - if len(formats) != numFormats { - t.Errorf("numFormats = %d, want %d as Formats returns", numFormats, len(formats)) - } - - for i, f := range formats { - if got := int(f.number()); got != i+1 { - t.Errorf("%s.number() = %d, want %d", f, got, i+1) - } - } - - for _, f := range []Format{"", "bogus", "ISO8601"} { - if got := f.number(); got != 0 { - t.Errorf("Format(%q).number() = %d, want 0", string(f), got) - } - } -} diff --git a/json.go b/json.go index 89eafe2..0777d60 100644 --- a/json.go +++ b/json.go @@ -3,6 +3,7 @@ package jsonduration import ( "encoding/json/jsontext" "encoding/json/v2" + "slices" "time" ) @@ -16,10 +17,11 @@ import ( // in object names and with json.StringifyNumbers. JSON null sets the duration to zero. // // The options are safe for concurrent use. -// With builds them on the first call and returns the same ones for the same arguments afterwards, -// so a call costs a lookup and no allocation, and it makes no difference whether you keep the options or ask again: +// Without decoding options, With returns the same ones on every call and allocates nothing, +// so it makes no difference whether you keep them or ask again. With decoding options it builds new ones, +// so keep those: // -// var jsonOpts = jsonduration.With(jsonduration.ISO8601) +// var jsonOpts = jsonduration.With(jsonduration.ISO8601, jsonduration.AlsoAccept(jsonduration.Nano)) // // b, err := json.Marshal(v, jsonOpts) // err = json.Unmarshal(b, &v, jsonOpts) @@ -45,7 +47,6 @@ func Unmarshalers(f Format, opts ...DecodeOption) *json.Unmarshalers { // [AlsoAccept] adds formats, and [AcceptDays] reads ISO 8601 days. Writing always uses the one format given. type DecodeOption interface { // apply returns the configuration with the option applied. - // It takes a value, not a pointer, so that the configuration of a call can stay on the stack. apply(decodeConfig) decodeConfig } @@ -62,17 +63,14 @@ type DecodeOption interface { // // The order of the formats makes no difference. AlsoAccept panics if a format is not valid. func AlsoAccept(formats ...Format) DecodeOption { - var accepted formatSet for _, f := range formats { - bit := f.bit() - if bit == 0 { + if !f.Valid() { panic(errInvalidFormat(f)) } - - accepted |= bit } - return alsoAccept(accepted) + // A copy, so that the option does not change if the caller changes the slice afterwards. + return alsoAccept(slices.Clone(formats)) } // AcceptDays makes [ISO8601] decoding read a day as exactly 24 hours and a week as 7 days: "P1DT12H" is 36h. @@ -86,9 +84,9 @@ func AcceptDays() DecodeOption { return acceptDays{} } -// decodeConfig is what the options of a call add up to. +// decodeConfig is what the options of a call add up to. accepted may name a format more than once, in any order. type decodeConfig struct { - accepted formatSet + accepted []Format acceptDays bool } @@ -104,11 +102,11 @@ func configFrom(opts []DecodeOption) decodeConfig { return config } -// alsoAccept is the option of [AlsoAccept]. It is one byte, which Go puts into an interface without allocating. -type alsoAccept formatSet +// alsoAccept is the option of [AlsoAccept]. +type alsoAccept []Format func (a alsoAccept) apply(c decodeConfig) decodeConfig { - c.accepted |= formatSet(a) + c.accepted = append(c.accepted, a...) return c } diff --git a/justfile b/justfile index edbf17b..1469e90 100644 --- a/justfile +++ b/justfile @@ -77,14 +77,6 @@ compat: compat-fuzz time="30s" *targets=all_fuzz_targets: for t in {{ targets }}; do go test -run='^$' -fuzz="^$t\$" -fuzztime={{ time }} . || exit 1; done -# The fuzz targets of both modules as a matrix for GitHub Actions: CI's fuzz job runs whatever this prints. -fuzz-matrix: - #!/usr/bin/env bash - set -euo pipefail - list() { go test -list '^Fuzz' . | grep '^Fuzz' | sed "s/$/ $1/"; } - { list fuzz; cd internal/compat; list compat-fuzz; } | - jq -R -c -s 'split("\n") | map(select(. != "") | split(" ") | {target: .[0], recipe: .[1]}) | {include: .}' - bench: go test -run='^$' -bench=. -benchmem ./... diff --git a/shared.go b/shared.go index b6f650e..6c7961f 100644 --- a/shared.go +++ b/shared.go @@ -4,66 +4,64 @@ import ( "encoding/json/jsontext" "encoding/json/v2" "reflect" - "sync/atomic" "time" ) // shared holds what [With], [Marshalers] and [Unmarshalers] return for one set of arguments. -// Its values never change and are safe for concurrent use, so every call with these arguments gets the same ones. -// That saves more than building them: encoding/json/v2 remembers per value which types it has seen. +// Its values never change and are safe for concurrent use. type shared struct { marshalers *json.Marshalers unmarshalers *json.Unmarshalers options json.Options } -// sharedByArgs holds the shared values of every set of arguments: by the number of the written format less one, -// by the set of the other accepted formats, and by whether days are accepted. -// A set of arguments that is not valid, such as one with two numeric formats, keeps its nil pointer. -// An array needs no hashing and no lock, and at 768 pointers it is small. -var sharedByArgs [numFormats][1 << numFormats][2]atomic.Pointer[shared] +// sharedByFormat holds the values of each format without decoding options, which is how most calls ask for them. +// They are built once, so that such a call allocates nothing and every caller gets the same ones: +// encoding/json/v2 remembers per value which types it has seen. +var sharedByFormat = func() map[Format]*shared { + byFormat := make(map[Format]*shared, len(Formats())) + for _, f := range Formats() { + byFormat[f] = newShared(f, decodeConfig{}, nil) + } -// sharedFor returns the shared values for the arguments of [With], and builds them on the first call. -// It panics as [With] does. -func sharedFor(f Format, opts []DecodeOption) *shared { - config := configFrom(opts) + return byFormat +}() - number := f.number() - if number == 0 { +// sharedFor returns the values for the arguments of [With]: the ones of the format if no option changes anything, +// and new ones otherwise. It panics as [With] does. +func sharedFor(f Format, opts []DecodeOption) *shared { + plain, ok := sharedByFormat[f] + if !ok { panic(errInvalidFormat(f)) } - // Accepting the written format again changes nothing, so it gets no entry of its own. - config.accepted &^= f.bit() - - days := 0 - if config.acceptDays { - days = 1 + config := configFrom(opts) + if len(config.accepted) == 0 && !config.acceptDays { + return plain } - slot := &sharedByArgs[number-1][config.accepted][days] - if s := slot.Load(); s != nil { - return s - } + // Options change only how durations are read, so the marshalers are those of the format alone. + return newShared(f, config, plain.marshalers) +} - // This panics for arguments that are not valid, before anything is stored. +// newShared builds the values for format f and the options of a call. Without marshalers, it builds those too. +// It panics if a second numeric format is accepted. +func newShared(f Format, config decodeConfig, marshalers *json.Marshalers) *shared { dc := newDecoder(f, config) dc.goType = reflect.TypeFor[time.Duration]() - s := &shared{unmarshalers: json.UnmarshalFromFunc(dc.decode)} - if config == (decodeConfig{}) { + if marshalers == nil { c := f.mustCodec() - s.marshalers = json.MarshalToFunc(func(enc *jsontext.Encoder, d time.Duration) error { + marshalers = json.MarshalToFunc(func(enc *jsontext.Encoder, d time.Duration) error { return encode(enc, c, d) }) - } else { - // Options change only how durations are read, so the marshalers are those of the format alone. - s.marshalers = sharedFor(f, nil).marshalers } - s.options = json.JoinOptions(json.WithMarshalers(s.marshalers), json.WithUnmarshalers(s.unmarshalers)) + unmarshalers := json.UnmarshalFromFunc(dc.decode) - // If another goroutine was faster, its values are the ones to share. - slot.CompareAndSwap(nil, s) - return slot.Load() + return &shared{ + marshalers: marshalers, + unmarshalers: unmarshalers, + options: json.JoinOptions(json.WithMarshalers(marshalers), json.WithUnmarshalers(unmarshalers)), + } } diff --git a/shared_test.go b/shared_test.go index aee1f2d..fe35f24 100644 --- a/shared_test.go +++ b/shared_test.go @@ -3,47 +3,40 @@ package jsonduration import ( "bytes" "encoding/json/v2" - "sync" "testing" "time" ) -// TestSameArgumentsShareOptions checks which calls get the same values: those that write the same format and read the -// same formats. It compares the unmarshalers, since they are pointers and the options are not comparable. -func TestSameArgumentsShareOptions(t *testing.T) { - base := Unmarshalers(ISO8601, AlsoAccept(Nano, Units)) - - same := map[string]*json.Unmarshalers{ - "again": Unmarshalers(ISO8601, AlsoAccept(Nano, Units)), - "other order": Unmarshalers(ISO8601, AlsoAccept(Units, Nano)), - "a format twice": Unmarshalers(ISO8601, AlsoAccept(Nano, Units, ISO8601, Nano)), - "two options": Unmarshalers(ISO8601, AlsoAccept(), AlsoAccept(Nano), AlsoAccept(Units)), - "a nil option": Unmarshalers(ISO8601, nil, AlsoAccept(Nano, Units)), - "the written one": Unmarshalers(ISO8601, AlsoAccept(ISO8601), AlsoAccept(Nano, Units)), - } - for name, got := range same { - if got != base { - t.Errorf("%s: got other unmarshalers, want the shared ones", name) +// TestFormatSharesOptions checks which calls get the same values: those that write the same format and have no +// option that changes anything. It compares the unmarshalers, since they are pointers and the options are not +// comparable. +func TestFormatSharesOptions(t *testing.T) { + for _, f := range Formats() { + if first, second := Marshalers(f), Marshalers(f); first != second { + t.Errorf("Marshalers(%s) differ between calls", f) } - } - different := map[string]*json.Unmarshalers{ - "other written format": Unmarshalers(Nano, AlsoAccept(ISO8601, Units)), - "no other formats": Unmarshalers(ISO8601), - "fewer formats": Unmarshalers(ISO8601, AlsoAccept(Nano)), - "other format": Unmarshalers(ISO8601, AlsoAccept(Sec, Units)), - "with days": Unmarshalers(ISO8601, AlsoAccept(Nano, Units), AcceptDays()), - } - for name, got := range different { - if got == base { - t.Errorf("%s: got the unmarshalers of ISO8601 with Nano and Units", name) + base := Unmarshalers(f) + + same := map[string]*json.Unmarshalers{ + "again": Unmarshalers(f), + "a nil option": Unmarshalers(f, nil), + "an empty option": Unmarshalers(f, AlsoAccept()), + } + for name, got := range same { + if got != base { + t.Errorf("%s, %s: got other unmarshalers, want the shared ones", f, name) + } } - } - for _, f := range Formats() { - first, second := Marshalers(f), Marshalers(f) - if first != second { - t.Errorf("Marshalers(%s) differ between calls", f) + different := map[string]*json.Unmarshalers{ + "another format": Unmarshalers(f, AlsoAccept(Units, ISO8601)), + "with days": Unmarshalers(f, AcceptDays()), + } + for name, got := range different { + if got == base { + t.Errorf("%s, %s: got the unmarshalers of the format alone", f, name) + } } } } @@ -61,9 +54,9 @@ func TestOptionsDoNotChangeMarshalers(t *testing.T) { } } -// TestWithDoesNotAllocate checks the promise in the documentation of With: asking again costs no allocation, -// with decoding options or without, so that decoding with options from a call allocates as little as with kept ones, -// which is not at all. +// TestWithDoesNotAllocate checks the promise in the documentation of With: without decoding options, asking again +// costs no allocation, so that decoding with options from a call allocates as little as with kept ones, which is not +// at all. func TestWithDoesNotAllocate(t *testing.T) { for _, f := range Formats() { in, err := json.Marshal(d90, With(f)) @@ -81,10 +74,7 @@ func TestWithDoesNotAllocate(t *testing.T) { {"Marshalers", 0, func() { _ = Marshalers(f) }}, {"Unmarshalers", 0, func() { _ = Unmarshalers(f) }}, {"With a nil option", 0, func() { _ = With(f, nil) }}, - {"With options", 0, func() { _ = With(f, AlsoAccept(Units, ISO8601), AcceptDays()) }}, - {"Unmarshalers with options", 0, func() { _ = Unmarshalers(f, AlsoAccept(Units, ISO8601), AcceptDays()) }}, {"Unmarshal", 0, func() { _ = json.Unmarshal(in, &d, With(f)) }}, - {"Unmarshal with options", 0, func() { _ = json.Unmarshal(in, &d, With(f, AlsoAccept(Units, ISO8601))) }}, } for _, tt := range tests { @@ -95,55 +85,6 @@ func TestWithDoesNotAllocate(t *testing.T) { } } -// TestInvalidArgumentsAreNotStored checks that a call that panics leaves nothing behind, -// so that the shared values cannot grow without bound, and that it panics again the next time. -func TestInvalidArgumentsAreNotStored(t *testing.T) { - count := func() int { - n := 0 - for i := range sharedByArgs { - for j := range sharedByArgs[i] { - for k := range sharedByArgs[i][j] { - if sharedByArgs[i][j][k].Load() != nil { - n++ - } - } - } - } - - return n - } - - // Build the valid part first: afterwards, an invalid call must not find it. - With(ISO8601) - before := count() - - invalid := map[string]func(){ - "unknown format": func() { With("bogus") }, - "empty format": func() { With("") }, - "unknown accepted": func() { With(ISO8601, AlsoAccept("bogus")) }, - "empty accepted": func() { With(ISO8601, AlsoAccept("")) }, - "two numeric": func() { With(Nano, AlsoAccept(Sec)) }, - "unknown after a second": func() { With(Nano, AlsoAccept(Sec, "bogus")) }, - } - - for name, fn := range invalid { - for range 2 { - func() { - defer func() { - if recover() == nil { - t.Errorf("%s: no panic", name) - } - }() - fn() - }() - } - } - - if after := count(); after != before { - t.Errorf("%d shared values after invalid calls, want %d as before", after, before) - } -} - // TestOptionValueIsReusable checks that an option used in several calls is not changed by the options after it. func TestOptionValueIsReusable(t *testing.T) { units := AlsoAccept(Units) @@ -166,26 +107,14 @@ func TestOptionValueIsReusable(t *testing.T) { if err := json.Unmarshal([]byte(inUnits), &d, onlyUnits); err != nil || d != d90 { t.Errorf("Unmarshal(%s) with units only = %v, %v, want %v", inUnits, d, err, d90) } -} - -// TestConcurrentFirstCalls only finds a data race when run with -race, as CI does. -// Every goroutine must get the same values, whichever built them. -func TestConcurrentFirstCalls(t *testing.T) { - // No other test asks for this set of arguments, so these calls are the first. - args := func() *json.Unmarshalers { return Unmarshalers(Micro, AlsoAccept(Units, ISO8601), AcceptDays()) } - - got := make([]*json.Unmarshalers, 16) - - var wg sync.WaitGroup - for i := range got { - wg.Go(func() { got[i] = args() }) - } - wg.Wait() + // The option keeps the formats it was given, also if the caller changes the slice afterwards. + formats := []Format{Units} + fromSlice := AlsoAccept(formats...) + formats[0] = ISO8601 - for i, u := range got { - if u != got[0] { - t.Errorf("goroutine %d got other unmarshalers than goroutine 0", i) - } + d = 0 + if err := json.Unmarshal([]byte(inUnits), &d, With(Sec, fromSlice)); err != nil || d != d90 { + t.Errorf("Unmarshal(%s) after the slice changed = %v, %v, want %v", inUnits, d, err, d90) } }