Conversation
WalkthroughRemoved the initialization of a hash variable in a macro in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
insert_string_tpl.h (2)
59-76: Optional: Mirror the local pointer/mask caching in QUICK_INSERT_STRING for consistencyThis function still dereferences
s->head,s->prev, ands->w_maskdirectly. While it’s not in a loop, it’s still on a hot path and aligning with the pattern you used in INSERT_STRING may tighten generated code slightly and improves consistency.Apply this refactor within the shown range:
Z_INTERNAL Pos QUICK_INSERT_STRING(deflate_state *const s, uint32_t str) { - uint8_t *strstart = s->window + str + HASH_CALC_OFFSET; + uint8_t *strstart = s->window + str + HASH_CALC_OFFSET; + Pos *headp = s->head; + Pos *prevp = s->prev; + unsigned int w_mask = s->w_mask; uint32_t val, hm; Pos head; HASH_CALC_VAR_INIT; HASH_CALC_READ; HASH_CALC(HASH_CALC_VAR, val); HASH_CALC_VAR &= HASH_CALC_MASK; hm = HASH_CALC_VAR; - head = s->head[hm]; + head = headp[hm]; if (LIKELY(head != str)) { - s->prev[str & s->w_mask] = head; - s->head[hm] = (Pos)str; + prevp[str & w_mask] = head; + headp[hm] = (Pos)str; } return head; }
89-92: Nit: Make w_mask const to express intent
w_maskis read-only. Declaring itconstdocuments intent and can sometimes help the compiler.- unsigned int w_mask = s->w_mask; + const unsigned int w_mask = s->w_mask;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
insert_string.c(1 hunks)insert_string_tpl.h(3 hunks)
🔇 Additional comments (2)
insert_string.c (1)
15-15: LGTM: Dropping the zero-init avoids a redundant store
HASH_CALC_VAR_INITno longer initializeshto 0. In this TU,his always assigned byHASH_CALC(HASH_CALC_VAR, val)before use, so this is safe and removes a needless instruction on hot paths.insert_string_tpl.h (1)
102-106: LGTM: Local aliasing for head/prev/mask removes s-> redirection in the loopThe switch to
headp,prevp, andw_maskinside the loop is correct and functionally identical. This should slightly reduce pointer chasing and improve instruction cache friendliness in this hot path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1951 +/- ##
==========================================
Coverage ? 81.93%
==========================================
Files ? 162
Lines ? 13926
Branches ? 3122
==========================================
Hits ? 11410
Misses ? 1518
Partials ? 998 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9d3ca79 to
190df18
Compare
Develop 14 Aug 2025PR opt insert_stringDeflatebench shows about 0.2% speedup. |
nmoinvaz
left a comment
There was a problem hiding this comment.
LGTM, might be a good idea to mention a comment about indirection like you did in your most recent PR.
190df18 to
e079160
Compare
Remove the
s->indirection penalty from the loop in insert_string for a very minor speedup in compression.Also some minor code cleanup.
Summary by CodeRabbit