Skip to content

[question] TtlCacheBacking - SystemTime or Instant? #7

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Closed
tsnoam opened this issue Sep 6, 2021 · 4 comments · Fixed by #8
Closed

[question] TtlCacheBacking - SystemTime or Instant? #7

tsnoam opened this issue Sep 6, 2021 · 4 comments · Fixed by #8

Comments

@tsnoam
Copy link

tsnoam commented Sep 6, 2021

Hi,

I was looking at this crate, evaluating it for my use.
While examining the code of TtlCacheBacking I've noticed that it's using SystemTime in order to determine the age of a cached entry.

My question is, why SystemTime instead of Instance? Was this due to a problem which you've encountered?

The reason for my questions is a caveat with SystemTime which each call to SystemTime::now() translates to a system call (at least on Linux) - a relatively slow operation.
Under heavy load, the affect of this can be significant.
Instant, on the other hand, does not suffer from the same problem.

@ByteAlex
Copy link
Owner

Sorry, I have not received any notification about this issue.
Using SystemTime was just a stupid approach of mine, I was not aware of the performance impact of it.
I'll move it to an Instant instead!

@ByteAlex
Copy link
Owner

About to leave, - tests were passing. If you don't mind give it a shot and let me know if you encounter any issues with it or if it's good to go! :)

@ByteAlex
Copy link
Owner

ByteAlex commented Oct 7, 2021

Since there was no action on this ticket for more than 2 weeks and the PR was sitting there waiting for a review, I've went ahead and merged this, since I did not observe any issues myself.
In case you've further questions, please don't hesitiate to reopen the issue.

Thank you!

@tsnoam
Copy link
Author

tsnoam commented Oct 7, 2021

Hi,
Sorry for the delay. I somehow missed it.
Yes, the change looks great.

Thankyou!

No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants