Skip to content

Conversation

@klauspost
Copy link
Owner

@klauspost klauspost commented Sep 19, 2025

Just use the dedicated function.

Summary by CodeRabbit

  • Refactor
    • Optimized the fast compression path by consolidating match-length calculations for fewer comparisons.
    • Removed an unused internal dependency, slightly reducing binary size.
    • Improves compression speed and reduces CPU usage, especially on large or repetitive data.
    • No changes to public APIs or compressed output; behavior remains compatible.
    • Error handling and control flow are unaffected.
    • No configuration changes required.

Just use the dedicated function.
@coderabbitai
Copy link

coderabbitai bot commented Sep 19, 2025

📝 Walkthrough

Walkthrough

Replaced custom match-length loops in fastGen.matchlen and fastGen.matchlenLong with direct calls to matchLen using constructed slices. Removed math/bits import no longer needed. No public APIs changed.

Changes

Cohort / File(s) Summary of Changes
Fast encoder match-length refactor
flate/fast_encoder.go
Replaced per-block 8-byte comparison in matchlen with matchLen(a, b) on sliced inputs; simplified matchlenLong to return matchLen(src[s:], src[t:]); removed unused math/bits import; retained existing control flow and error handling; no public API changes.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as fast encoder
    participant FE as fastGen
    participant ML as matchLen

    rect rgb(232, 244, 253)
    note over FE: Updated flow
    Caller->>FE: matchlen(src, s, t)
    FE->>ML: matchLen(src[s:limit], src[t:])
    ML-->>FE: length
    FE-->>Caller: int32(length)
    end

    rect rgb(237, 247, 237)
    note over FE: Updated flow
    Caller->>FE: matchlenLong(src, s, t)
    FE->>ML: matchLen(src[s:], src[t:])
    ML-->>FE: length
    FE-->>Caller: int32(length)
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "flate: Simplify matchlen" is concise and accurately summarizes the primary change by naming the affected package and the intent to simplify the matchlen implementation. It directly reflects the raw_summary (replacing manual per-block comparison and long-match logic with calls to matchLen) and aligns with the PR objectives. The phrasing is specific and clear enough for a reviewer scanning history to understand the main change.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch flate-simplify-matchlen

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@klauspost klauspost changed the title Simplify matchlen flate: Simplify matchlen Sep 19, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
flate/fast_encoder.go (1)

153-156: Good simplification; consider clamping instead of slicing.

Calling matchLen is the right direction. To avoid the extra slice creation, you can call matchLen on the full tails and clamp the result to maxMatchLength-4.

- a := src[s:min(s+maxMatchLength-4, len(src))]
- b := src[t:]
- return int32(matchLen(a, b))
+ l := matchLen(src[s:], src[t:])
+ if l > maxMatchLength-4 {
+   l = maxMatchLength - 4
+ }
+ return int32(l)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 54cb7a5 and 418313b.

📒 Files selected for processing (1)
  • flate/fast_encoder.go (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
  • GitHub Check: build (1.25.x, macos-latest)
  • GitHub Check: build (1.25.x, windows-latest)
  • GitHub Check: build (1.24.x, ubuntu-latest)
  • GitHub Check: build (1.25.x, ubuntu-latest)
  • GitHub Check: build (1.24.x, macos-latest)
  • GitHub Check: build (1.23.x, windows-latest)
  • GitHub Check: build (1.23.x, macos-latest)
  • GitHub Check: build (1.23.x, ubuntu-latest)
  • GitHub Check: build (1.24.x, windows-latest)
  • GitHub Check: fuzz-zstd ("noasm,nounsafe")
  • GitHub Check: fuzz-other ("noasm,nounsafe")
  • GitHub Check: fuzz-other (nounsafe)
  • GitHub Check: fuzz-s2 ("noasm,nounsafe")
  • GitHub Check: fuzz-zstd (nounsafe)
  • GitHub Check: fuzz-s2 (nounsafe)
  • GitHub Check: build-special
🔇 Additional comments (1)
flate/fast_encoder.go (1)

175-176: LGTM: direct use of matchLen for long matches.

This is clearer and should leverage the optimized implementation.

@klauspost klauspost merged commit f44517c into master Sep 19, 2025
22 checks passed
@klauspost klauspost deleted the flate-simplify-matchlen branch September 19, 2025 10:05
hantang pushed a commit to qundao/mirror-forgejo that referenced this pull request Oct 21, 2025
This PR contains the following updates:

| Package | Change | Age | Confidence |
|---|---|---|---|
| [github.com/klauspost/compress](https://github.com/klauspost/compress) | `v1.18.0` -> `v1.18.1` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fklauspost%2fcompress/v1.18.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fklauspost%2fcompress/v1.18.0/v1.18.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>klauspost/compress (github.com/klauspost/compress)</summary>

### [`v1.18.1`](https://github.com/klauspost/compress/releases/tag/v1.18.1)

[Compare Source](klauspost/compress@v1.18.0...v1.18.1)

#### What's Changed

- zstd: Fix incorrect buffer size in dictionary encodes by [@&#8203;klauspost](https://github.com/klauspost) in [#&#8203;1059](klauspost/compress#1059)
- s2: check for cap, not len of buffer in EncodeBetter/Best by [@&#8203;vdarulis](https://github.com/vdarulis) in [#&#8203;1080](klauspost/compress#1080)
- zstd: Add simple zstd EncodeTo/DecodeTo functions by [@&#8203;klauspost](https://github.com/klauspost) in [#&#8203;1079](klauspost/compress#1079)
- zlib: Avoiding extra allocation in zlib.reader.Reset by [@&#8203;travelpolicy](https://github.com/travelpolicy) in [#&#8203;1086](klauspost/compress#1086)
- gzhttp: remove redundant err check in zstdReader by [@&#8203;ryanfowler](https://github.com/ryanfowler) in [#&#8203;1090](klauspost/compress#1090)
- Run modernize. Deprecate Go 1.22 by [@&#8203;klauspost](https://github.com/klauspost) in [#&#8203;1095](klauspost/compress#1095)
- flate: Simplify matchlen by [@&#8203;klauspost](https://github.com/klauspost) in [#&#8203;1101](klauspost/compress#1101)
- flate: Add examples by [@&#8203;klauspost](https://github.com/klauspost) in [#&#8203;1102](klauspost/compress#1102)
- flate: Use exact sizes for huffman tables by [@&#8203;klauspost](https://github.com/klauspost) in [#&#8203;1103](klauspost/compress#1103)
- flate: Faster load+store by [@&#8203;klauspost](https://github.com/klauspost) in [#&#8203;1104](klauspost/compress#1104)
- Add notice to S2 about MinLZ by [@&#8203;klauspost](https://github.com/klauspost) in [#&#8203;1065](klauspost/compress#1065)

#### New Contributors

- [@&#8203;wooffie](https://github.com/wooffie) made their first contribution in [#&#8203;1069](klauspost/compress#1069)
- [@&#8203;vdarulis](https://github.com/vdarulis) made their first contribution in [#&#8203;1080](klauspost/compress#1080)
- [@&#8203;travelpolicy](https://github.com/travelpolicy) made their first contribution in [#&#8203;1086](klauspost/compress#1086)
- [@&#8203;ryanfowler](https://github.com/ryanfowler) made their first contribution in [#&#8203;1090](klauspost/compress#1090)

**Full Changelog**: <klauspost/compress@v1.18.0...v1.18.1>

</details>

---

### Configuration

📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS4xNTIuOSIsInVwZGF0ZWRJblZlciI6IjQxLjE1Mi45IiwidGFyZ2V0QnJhbmNoIjoiZm9yZ2VqbyIsImxhYmVscyI6WyJkZXBlbmRlbmN5LXVwZ3JhZGUiLCJ0ZXN0L25vdC1uZWVkZWQiXX0=-->

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9786
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Co-authored-by: Renovate Bot <forgejo-renovate-action@forgejo.org>
Co-committed-by: Renovate Bot <forgejo-renovate-action@forgejo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants