Skip to content

Avoid memory allocation for hash object operations wherever possible #1248

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

Merged
merged 9 commits into from
Jun 23, 2025

Conversation

Mimi8298
Copy link
Contributor

Currently, most operations on Hash Objects allocate a key value to lookup the stored value. The aim here is to:

  • avoid all memory allocations when the value already exists in the object (>= net.9)
  • reuse the key instance for expireTime (>= net.9)
  • avoid allocating a new value array when replacing a value of the same size

@Copilot Copilot AI review requested due to automatic review settings June 11, 2025 15:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes hash object operations by conditionally using ReadOnlySpan-based implementations (for NET9_0_OR_GREATER) to avoid unnecessary memory allocations and reuse existing memory buffers where possible. Key changes include switching from ToByteArray() to AsReadOnlySpan() for key extraction, conditionally copying values only when their sizes differ, and updating method signatures to use a unified ByteSpan type.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
libs/server/Objects/Hash/HashObjectImpl.cs Updated key/value extraction and memory reuse logic with conditional compilation for NET9_0_OR_GREATER.
libs/server/Objects/Hash/HashObject.cs Modified API methods to use the new ByteSpan type and alternate lookup mechanisms.
libs/common/RespMemoryWriter.cs Changed WriteIntegerFromBytes to accept a ReadOnlySpan instead of a byte array.

@PaulusParssinen
Copy link
Contributor

PaulusParssinen commented Jun 12, 2025

Didn't think of using type alias for the key argument, makes this much cleaner than my previous attempt 👍

@Mimi8298
Copy link
Contributor Author

@PaulusParssinen After the PR merge, maybe you can also commit your multi lookup optimization?

@Mimi8298 Mimi8298 requested a review from prvyk June 12, 2025 16:46
@TalZaccai TalZaccai requested review from TalZaccai and removed request for prvyk June 12, 2025 18:09
Copy link
Contributor

@TalZaccai TalZaccai left a comment

Choose a reason for hiding this comment

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

Functionally looks good to me, perhaps needs some slight tweaks to make code more readable. LMK what you think.

@TalZaccai TalZaccai merged commit be243a7 into microsoft:main Jun 23, 2025
26 checks passed
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.

4 participants