Skip to content

Conversation

@guybe7
Copy link
Collaborator

@guybe7 guybe7 commented Feb 6, 2020

No description provided.

@antirez
Copy link
Contributor

antirez commented Feb 20, 2020

Hello @guybe7, I wonder if this makes any measurable difference. In theory we could do that in many places, but that would make the code a bit more complex than needed. So why doing that only for streams?

@guybe7
Copy link
Collaborator Author

guybe7 commented Apr 1, 2020

hi @antirez
TBH i'm not sure it is has any measurable difference, i was just going over the code in t_stream and it caught my eye... feel free to close it if you want

@antirez
Copy link
Contributor

antirez commented Apr 2, 2020

@guybe7 the PR is fine, but I want to close it since it's a long story that must be addressed at once IMHO. We should have access to different mechanisms. I want to given an example of what we can do:

  • mstime() -> This should be used when we really need the current time.
  • loop_mstime() -> This should be a cached value that we refresh always in afterSleep(): this way we know it is not an outdated information. This should replace all the accesses to server.mstime that we do now when we don't need a very fresh time.

On top of that, both these calls should no longer use gettimeofday() but a monolithic clock. Then we should use one call or the other, depending on how much freshness we really require. But I bet we'll be able many many times to use the cached value, because the value cached in this event loop cycle is fresh enough for a number of uses.

Moreover, we should check RTSC that is in new processors finally fixed about the multi core issue that plagued it for ages, in order to use that instead in many places, or even more interestingly, just to provide a very powerful cached_mstime() that is able to update it automatically when the RTSC timer signals that there is too much delta compared to when we refreshed it.

So this is a thing that we should handle in a proper way during the start of a major release, like in Redis 7.

Maybe we should copy that in an issue?

@guybe7
Copy link
Collaborator Author

guybe7 commented Apr 2, 2020

@antirez ok, i'll open an issue and cite your reply in it

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