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.

Sign up for GitHub

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

Jump to bottom

Correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. #476

Merged
merged 2 commits into from May 16, 2024
Merged

Correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. #476

merged 2 commits into from May 16, 2024
+38 −26

Conversation

lipzhu
Copy link
Contributor

@lipzhu lipzhu commented May 9, 2024 •
edited

This patch try to correct the actual allocated size from allocator
when call sdsRedize to align the logic with sdsnewlen function.

Maybe the #453 optimization should depend on this.

@lipzhu lipzhu force-pushed the sds_header branch 2 times, most recently from 1a73354 to 9b30c8a Compare May 9, 2024 06:20
@codecov Codecov
Copy link

codecov bot commented May 9, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.84%. Comparing base (546cef6) to head (41f32de).
Report is 1 commits behind head on unstable.

Additional details and impacted files 
@@            Coverage Diff            @@
##           unstable     #476   +/-   ##
=========================================
  Coverage     69.84%   69.84%           
=========================================
  Files           109      109           
  Lines         61797    61801    +4     
=========================================
+ Hits          43160    43163    +3     
- Misses        18637    18638    +1
Files Coverage Δ
src/sds.c 87.01% <100.00%> (+0.09%) ⬆️

... and 11 files with indirect coverage changes

@poiuj
Copy link

poiuj commented May 9, 2024 •
edited

Right, I did very similar changes. Didn't push yet.

@lipzhu
Correct sds header with actual allocated size.
9b30c8a 
Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com>
@lipzhu lipzhu mentioned this pull request May 9, 2024
Merged
@lipzhu lipzhu changed the title Correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function [Fix]Correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. May 9, 2024
@lipzhu lipzhu changed the title [Fix]Correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. Correct the actual allocated size from allocator when call sdsRedize to align the logic with sdsnewlen function. May 9, 2024
madolson
madolson reviewed May 14, 2024
View reviewed changes
src/sds.c Outdated Show resolved Hide resolved
madolson
madolson reviewed May 14, 2024
View reviewed changes
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

This looks good to me, can you move the tests to the new framework?

@lipzhu
merge unstable
41f32de 
Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com>
@poiuj poiuj mentioned this pull request May 15, 2024
Open
@lipzhu
Copy link
Contributor Author

lipzhu commented May 16, 2024

This looks good to me, can you move the tests to the new framework?

Done.

madolson
madolson approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@madolson madolson left a comment

Choose a reason for hiding this comment

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

A nice to have, but not necessarily in the scope of this PR, is we should probably add more checks that sdsalloc() is actually always equal to the underlying zmalloc allocation size.

@madolson madolson merged commit 7a9951f into valkey-io:unstable May 16, 2024
17 checks passed
@lipzhu lipzhu deleted the sds_header branch May 16, 2024 01:30
@poiuj
Copy link

poiuj commented May 16, 2024 •
edited

Just to clarify, underlying allocation size equals to sdsalloc() + header size + null terminator. sdsAllocSize() should calculate it.

@madolson madolson requested review from ranshid and removed request for ranshid May 16, 2024 16:53
@madolson
Copy link
Member

madolson commented May 19, 2024 •
edited

@lipzhu A test started to fail after we merged this change related to allocated size:
https://github.com/valkey-io/valkey/actions/runs/9143545732/job/25140329542#step:6:1505
https://github.com/valkey-io/valkey/actions/runs/9135654696/job/25123330670#step:6:1501

It's the libc malloc function, let me know if you think it's related and if you can try to fix it.

srgsanky pushed a commit to srgsanky/valkey that referenced this pull request May 19, 2024
@lipzhu @srgsanky
Correct the actual allocated size from allocator when call sdsRedize …
f76ae1e 
…to align the logic with sdsnewlen function. (valkey-io#476)

This patch try to correct the actual allocated size from allocator
when call sdsRedize to align the logic with sdsnewlen function.

Maybe the valkey-io#453 optimization
should depend on this.

Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com>
@lipzhu
Copy link
Contributor Author

lipzhu commented May 20, 2024

@madolson Sure, I will take a look and try to reproduce in local.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madolson madolson madolson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lipzhu @poiuj @madolson