Remove unused valDup #443
Remove unused valDup #443
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #443 +/- ##
===========================================
Coverage ? 68.45%
===========================================
Files ? 109
Lines ? 61680
Branches ? 0
===========================================
Hits ? 42221
Misses ? 19459
Partials ? 0
|
|
As mentioned on slack, can you run the performance test again with more keys and post the updated performance here. As long as there is a meaningful difference, I think it would be worth pulling it. |
|
For random key and small packets we will see some improvement. For larger packet the improvement is overshadowed by the overall load and is no longer measurable (though logic say it must be there however small). |
|
@eliblight Can you fix the DCO complaint? |
Signed-off-by: Eran Liberty <eran.liberty@gmail.com>
eliblight
force-pushed
the
try-to-remove-dup
branch
from
a956b50
to
b839456
Compare
| @@ -62,18 +62,6 @@ static unsigned int callbackHash(const void *key) { | |||
| hi_sdslen((const hisds)key)); | |||
| } | |||
|
|
|||
| static void *callbackValDup(void *privdata, const void *src) { | |||
There was a problem hiding this comment.
Let's not touch hiredis. We are looking to devendor it anyways, so I think they can make independent decisions about this.
There was a problem hiding this comment.
I think we must.
i.e. I changed the type and then changed whatever did not compile.
will recheck if we must and remove if not
There was a problem hiding this comment.
You shouldn't need to change hiredis. Hiredis is compiled independently as a static file and then linked against.
| @@ -820,9 +820,9 @@ void dictSetKey(dict *d, dictEntry* de, void *key) { | |||
| de->key = key; | |||
| } | |||
|
|
|||
| void dictSetVal(dict *d, dictEntry *de, void *val) { | |||
| void dictSetVal(dictEntry *de, void *val) { | |||
There was a problem hiding this comment.
| void dictSetVal(dictEntry *de, void *val) { | |
| void dictSetVal(dict *d, dictEntry *de, void *val) { | |
| UNUSED(d); |
Although a little silly, this makes the actual scope of the change much smaller. If we end up reverting it, we'll preserve a lot more history.
There was a problem hiding this comment.
Will need to inspect the assembly to say for sure but, this might force the compile to put "d" on the stack for nothing. It might force higher function that want to setVal to fetch the dict for nothing both in code lines and in assembly instruction (i.e. I am not sure the compiler will be able to optimize it out)
think about this and if you still prefer me to take your suggestion, say so and I will comply.
|
Sorry for taking a bit to get back around to this, can you address the merge conflicts and update the PR? I'm happy with it otherwise. |
| @@ -820,9 +820,9 @@ void dictSetKey(dict *d, dictEntry* de, void *key) { | |||
| de->key = key; | |||
| } | |||
|
|
|||
| void dictSetVal(dict *d, dictEntry *de, void *val) { | |||
There was a problem hiding this comment.
I like asserts during functional testing, but not so much in production. dictSetVal is now
void dictSetVal(dictEntry *de, void *val) {
assert(entryHasValue(de));
de->v.val = val;
}
Can we disable this assert except when testing? Can we take the next step and make this function in-line?
There was a problem hiding this comment.
Trust in the compiler! or "respect and suspect" and actually examine the assembly. But from my experience the compiler is way smarter than mere humans and will inline this and a lot more where it make sense.
There was a problem hiding this comment.
If cross-file optimization is turned on, then I would expect a good compiler to automatically inline this. We have a lot of inline directives in our code, I cannot tell you why.
I am more interested in changing assert to debugServerAssertWithInfo so that it is a no-op in production.
I have a zero-trust policy. I am willing to work with a "trust and verify" policy on the grounds that it really means "don't tell someone you don't trust them until after you have proof". I think that is almost equivalent to your "respect and suspect".
It turns out valDup is totally unused in the codebase