Skip to content
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

Code cleanup in object.c to support future work #1659

Closed
wants to merge 2 commits into from

Conversation

SoftlyRaining
Copy link
Contributor

Summary of changes:

  • Define/allocate layout of robj's embedded data in once place instead of two
  • More consistent naming: o to refer to robj, val/ptr/val_ptr to refer to contained value
  • Add static to helpers to clarify scope/usage and improve compiler optimization

My primary goal is to make the structure and memory layout of robj easier to change and work with in the future. This will support future work like eliminating the 8 byte ptr when value is embedded directly, and potentially expand value embedding beyond OBJ_ENCODING_EMBSTR and OBJ_ENCODING_INT.

I'm a bit unsure about the function signature of createObjectWithEmbeddedData - let me know if you have a cleaner idea!

Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 95.58824% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.03%. Comparing base (78bcc0a) to head (019d387).
Report is 9 commits behind head on unstable.

Files with missing lines Patch % Lines
src/object.c 95.58% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #1659   +/-   ##
=========================================
  Coverage     71.02%   71.03%           
=========================================
  Files           121      121           
  Lines         65176    65233   +57     
=========================================
+ Hits          46290    46336   +46     
- Misses        18886    18897   +11     
Files with missing lines Coverage Δ
src/server.h 100.00% <ø> (ø)
src/object.c 82.05% <95.58%> (+0.01%) ⬆️

... and 25 files with indirect coverage changes

…: o to refer to robj, val/ptr/val_ptr to refer to object's value. Add static to helpers

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Signed-off-by: Rain Valentine <rsg000@gmail.com>
@SoftlyRaining
Copy link
Contributor Author

I'm combining this with #1658 I think it'll be cleaner

robj *objectSetExpire(robj *val, long long expire);
sds objectGetKey(const robj *val);
long long objectGetExpire(const robj *val);
robj *createObjectWithEmbeddedData(int type, void *val_ptr, const sds key, long long expire, size_t val_embedded_size, size_t *val_embed_capacity);
Copy link
Contributor

Choose a reason for hiding this comment

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

The deduplication is good. Thanks for that.

I don't think we should expose this function. It's more complex than what the caller needs. So far, embedded strings are created using createStringObject with size checks inside of object.c to decide to embed or not. We can expose createStringObjectWithKeyAndExpire too if you want.

For non-strings, I think we should still expose createObjectWithKeyAndExpire and make it a thin wrapper of createObjectWithEmbeddedData which can be internal.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point. It makes sense not to expose that complex function signature. For now I think it makes sense not to expose functions we don't actually use outside of object.c - we can always expose them later when we need them.

It kind of makes sense to have createObjectWithKeyAndExpire, but it isn't used anywhere. Do we have a near-future use for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, if it's not used, let's not expose it.

I have a local branch where I want to create the object with expire from the start in commands like SETEX, but I don't think I'll prioritize it soon.

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