-
Notifications
You must be signed in to change notification settings - Fork 352
flate: Simplify matchlen #1101
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
flate: Simplify matchlen #1101
Conversation
Just use the dedicated function.
📝 WalkthroughWalkthroughReplaced 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
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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
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. Comment |
There was a problem hiding this 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
📒 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.
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` | [](https://docs.renovatebot.com/merge-confidence/) | [](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 [@​klauspost](https://github.com/klauspost) in [#​1059](klauspost/compress#1059) - s2: check for cap, not len of buffer in EncodeBetter/Best by [@​vdarulis](https://github.com/vdarulis) in [#​1080](klauspost/compress#1080) - zstd: Add simple zstd EncodeTo/DecodeTo functions by [@​klauspost](https://github.com/klauspost) in [#​1079](klauspost/compress#1079) - zlib: Avoiding extra allocation in zlib.reader.Reset by [@​travelpolicy](https://github.com/travelpolicy) in [#​1086](klauspost/compress#1086) - gzhttp: remove redundant err check in zstdReader by [@​ryanfowler](https://github.com/ryanfowler) in [#​1090](klauspost/compress#1090) - Run modernize. Deprecate Go 1.22 by [@​klauspost](https://github.com/klauspost) in [#​1095](klauspost/compress#1095) - flate: Simplify matchlen by [@​klauspost](https://github.com/klauspost) in [#​1101](klauspost/compress#1101) - flate: Add examples by [@​klauspost](https://github.com/klauspost) in [#​1102](klauspost/compress#1102) - flate: Use exact sizes for huffman tables by [@​klauspost](https://github.com/klauspost) in [#​1103](klauspost/compress#1103) - flate: Faster load+store by [@​klauspost](https://github.com/klauspost) in [#​1104](klauspost/compress#1104) - Add notice to S2 about MinLZ by [@​klauspost](https://github.com/klauspost) in [#​1065](klauspost/compress#1065) #### New Contributors - [@​wooffie](https://github.com/wooffie) made their first contribution in [#​1069](klauspost/compress#1069) - [@​vdarulis](https://github.com/vdarulis) made their first contribution in [#​1080](klauspost/compress#1080) - [@​travelpolicy](https://github.com/travelpolicy) made their first contribution in [#​1086](klauspost/compress#1086) - [@​ryanfowler](https://github.com/ryanfowler) made their first contribution in [#​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>
Just use the dedicated function.
Summary by CodeRabbit