Enhancemant Request
Source locating at: src/util.c, Line: 999.
No need define a variable for struct timeval, referring to manual as below:
DESCRIPTION
The system's notion of the current Greenwich time and the current time zone is obtained with the gettimeofday() call, and set with
the settimeofday() call. The time is expressed in seconds and microseconds since midnight (0 hour), January 1, 1970. The resolution
of the system clock is hardware dependent, and the time may be updated continuously or in ``ticks.'' If tp is NULL and tzp is non-
NULL, gettimeofday() will populate the timezone struct in tzp. If tp is non-NULL and tzp is NULL, then only the timeval struct in tp
is populated. If both tp and tzp are NULL, nothing is returned.
Describe the solution
Use an alternative call and specify NULL for tp.
e.g.
gettimeofday (NULL, &tz);
Comment From: sundb
thanks, welcome to make a PR.
Comment From: Bannirui
@sundb Okay, this is my first Redis PR, appreciate for your help if review need. PTAL.
Comment From: oranagra
i'm not sure i understand. is it faster this way? or just an unnecessary argument? i.e simplifying the code has a cost too (same as any change)
Comment From: Bannirui
i'm not sure i understand. is it faster this way? or just an unnecessary argument? i.e simplifying the code has a cost too (same as any change)
uhh,
firstly, for code perfermance, i have no idea whether it is faster by this way, maybe they almost have same perfermance.
cuz:
- code1 snippets, for gettimeofday (&tv, &tz)
asm
subq $32, %rsp
leaq -24(%rbp), %rsi
leaq -16(%rbp), %rdi
callq _gettimeofday
- code2 snippets, for gettimeofday (NULL, &tz)
asm
subq $16, %rsp
leaq -8(%rbp), %rsi
xorl %eax, %eax
movl %eax, %edi
callq _gettimeofday
so, i do not think they have obvious diff about invoker perfermance.
the second, i open this issue just because it confused me why should we define a variable struct timeval tv which never been used in functon scope. that's all. so i try to start PR to discard the useless code and make it more clean.
lastly, i'm sry for that i did not consider the cost behind code simplifing.
Comment From: sundb
@oranagra In theory, this PR would be faster, as the implementation in https://github.com/torvalds/linux/blob/d2f51b3516dade79269ff45eae2a7668ae711b25/kernel/time/time.c#L140-L156.
However, getTimeZone(_ is only called once, so this PR is just cleaning up the argument.
Comment From: oranagra
ok, the kernel code is indeed more optimal when this argument isn't provided, and in theory, this function may be called in other places in the future (although i highly doubt it, since there's a reason why we only call it on startup). the cost of code changes is unexpected bugs, and also merge conflicts for old PRs and other branches, but in this case it doesn't seem like a real concern, so let's merge this. please just make sure the PR top comment mentions the relevant info (what it does, and why). thanks.
Comment From: Bannirui
ok, the kernel code is indeed more optimal when this argument isn't provided, and in theory, this function may be called in other places in the future (although i highly doubt it, since there's a reason why we only call it on startup). the cost of code changes is unexpected bugs, and also merge conflicts for old PRs and other branches, but in this case it doesn't seem like a real concern, so let's merge this. please just make sure the PR top comment mentions the relevant info (what it does, and why). thanks.
thank you both, and thanks for explaining the concerns of code merging.
furthermore, i'm a little bit fresh to PR, what should i do, e,g, to re-open a new PR with revelent info(what it does, and why) that you mentationed above, or some of you will help modify/correct commit message while approving the PR request.
thanks again
Comment From: sundb
@Bannirui No need for a new pull request, just edit the top comment of this PR.
Comment From: Bannirui
@Bannirui No need for a new pull request, just edit the top comment of this PR.
got it, and i have editted the comment, appreciate for your guidence.
Comment From: imba-tjd
This change causes redis to crash on msys2 with no stackdump.
Though, the redis code doesn't need changing. It's a bug of cygwin. They didn't handle NULL for the first argument of gettimeofday. I will send them a email. Edit: fixed in https://github.com/cygwin/cygwin/commit/70653fd8f1db70b8776d22517fa737338eea18c7
If you want to build in such environment, as a workaround, you can change the code back, or use this script: sed -i '/getTimeZone(void)/a return timezone;' src/util.c
Note this change isn't related to the crash of redis 7.2 series. That crash leaves stackdump file containing Exception: STATUS_ACCESS_VIOLATION. Since that issue doesn't exist on unstable now, I won't work on it.
As of the build result. The build success, but the test fails, and 7.0.15 also fails at the same place: Testing unit/expire [exception]: Executing test client: error reading "socka001118e0": software caused connection abort.. I think it's ok for self-test on win.