Skip to content

Conversation

@jschmieg
Copy link

@jschmieg jschmieg commented Jul 2, 2020

This change is Reviewable

@jschmieg jschmieg force-pushed the optimSdsFree branch 2 times, most recently from 2d9e388 to 6b5165f Compare July 3, 2020 10:45
@jschmieg jschmieg requested a review from michalbiesek July 3, 2020 10:46
@jschmieg jschmieg marked this pull request as ready for review July 3, 2020 10:53
Copy link

@michalbiesek michalbiesek left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @jschmieg and @michalbiesek)


src/object.c, line 289 at r1 (raw file):

    }
}

nit: remove one newline - one is sufficient here


src/sds.c, line 192 at r1 (raw file):

static inline int sdsHdrSizeOptim(char* s) {
    switch((uintptr_t)s%8) {
	case 1:

please use spaces instead of tabs. Can we move this function upper see that inline functions are defined in the beginning?

@jschmieg jschmieg force-pushed the optimSdsFree branch 2 times, most recently from 449a6ba to ef35930 Compare July 3, 2020 11:11
@jschmieg jschmieg requested a review from michalbiesek July 3, 2020 11:11
Copy link

@michalbiesek michalbiesek left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @jschmieg and @michalbiesek)


src/sds.c, line 66 at r2 (raw file):

%8

Instead of "%8" can we use "&7" the result is the same but the second form is more efficient. The compiler with optimization will probably turn this in the same code but if you see e.g. siphash.c file there you have usage of the version with the bitwise operator and I prefer not to rely on the compiler itself as this is a pretty common pattern

Eliminates cache-miss on free of String. SdsHdrSize is retieved from
calculating %8 operation on sds pointer. Added padding in 2 Sds Headers
to have unique value of %8 operation.
Copy link

@michalbiesek michalbiesek left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r1, 2 of 2 files at r2, 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@michalbiesek michalbiesek merged commit 5fb70a6 into pmem:6.0-memkind_alloc_by_size Aug 17, 2020
PatKamin added a commit to PatKamin/redis that referenced this pull request Oct 28, 2022
[5.0.0-devel] Fix make test failing
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