Skip to content

Cleanup redundant incrRefCount and fix new_argv alloc size around HDEL#4059

Merged
enjoy-binbin merged 1 commit into
valkey-io:unstablefrom
enjoy-binbin:cleanup
Jun 29, 2026
Merged

Cleanup redundant incrRefCount and fix new_argv alloc size around HDEL#4059
enjoy-binbin merged 1 commit into
valkey-io:unstablefrom
enjoy-binbin:cleanup

Conversation

@enjoy-binbin

@enjoy-binbin enjoy-binbin commented Jun 26, 2026

Copy link
Copy Markdown
Member

Two minor cleanups:

  • Remove unnecessary incrRefCount(shared.hdel) just like other shared object.
  • Fix new_argv allocation size since HDEL only need (num_fields + 2) size.

@enjoy-binbin
Cleanup redundant incrRefCount and fix new_argv alloc size around HDEL
7aabd35 
Two minor cleanups:
- Remove unnecessary incrRefCount(shared.hdel) just like other shared object.
- Fix new_argv allocation size since HDEL only need (num_fields + 2) size.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin enjoy-binbin requested a review from ranshid June 26, 2026 08:15
@coderabbitai

coderabbitai Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3b125c2a-557e-4b66-a842-e9ffaeafb55e

📥 Commits

Reviewing files that changed from the base of the PR and between d38ff73 and 7aabd35.

📒 Files selected for processing (1)
  • src/t_hash.c
📝 Walkthrough

Walkthrough

This PR adjusts two hash expiration rewrite paths so the generated HDEL argument vector uses a smaller allocation and no longer increments shared.hdel before adding the key.

Changes

Hash expiration rewrite paths

Layer / File(s) Summary
Rewrite vector allocation
src/t_hash.c		 hsetexCommand and hexpireGenericCommand both change their HDEL rewrite argument construction to allocate one fewer slot and omit the shared.hdel refcount increment before appending the hash key.

Sequence Diagram(s)

Estimated Code Review Effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the two HDEL-related cleanup fixes in the changeset.
Description check ✅ Passed The description accurately describes the same HDEL refcount and allocation-size changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.73%. Comparing base (d38ff73) to head (7aabd35).

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #4059      +/-   ##
============================================
- Coverage     76.79%   76.73%   -0.06%     
============================================
  Files           162      162              
  Lines         81026    81024       -2     
============================================
- Hits          62222    62172      -50     
- Misses        18804    18852      +48
Files with missing lines Coverage Δ
src/t_hash.c 95.35% <100.00%> (-0.09%) ⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hpatro
hpatro approved these changes Jun 26, 2026
View reviewed changes

@hpatro hpatro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. incrRefCount has no effect on shared objects, obvious for Valkey developer, not for AI model I guess. We could figure out how to pass more context to AI model about Valkey codebase.

@enjoy-binbin enjoy-binbin merged commit 689906a into valkey-io:unstable Jun 29, 2026
65 checks passed
@enjoy-binbin enjoy-binbin deleted the cleanup branch June 29, 2026 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hpatro hpatro hpatro approved these changes
@ranshid ranshid Awaiting requested review from ranshid

Assignees

@enjoy-binbin enjoy-binbin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@enjoy-binbin @hpatro