Skip to content

Make Time public #2497

New issue

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

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

Closed
wants to merge 1 commit into from
Closed

Conversation

benthecarman
Copy link
Contributor

If we make Time public, this allows for other libraries to implement the handling of time themselves. This could be nice for things like wasm where we can't use the standard library to get the time, but we sitll have access to the time through other libraries.

@TheBlueMatt
Copy link
Collaborator

TheBlueMatt commented Aug 14, 2023

Hmm, I'm certainly not against exposing the time trait to let folks implement it themselves, but this by itself won't let someone compile for wasm as-is - there's plenty of other places in the library where std implies that std::time is available (in fact, its almost the only thing that std implies). We'd have to bound ~everything in the library that has any time lookup with the trait.

@benthecarman
Copy link
Contributor Author

benthecarman commented Aug 14, 2023

Yeah it won't help everywhere but at least in some places it can be used, for example the scorer

@dunxen
Copy link
Contributor

dunxen commented Aug 15, 2023

Concept ACK

@TheBlueMatt
Copy link
Collaborator

At least for the scorer, there's also an alternative way to go - give it timer ticks like we do ChannelManager/PeerManager. The nice thing about going that way is it actually reduces the amount of work we have to do during scoring, and can make routing faster as we no longer have to check + decay bounds during scoring.

If we make `Time` public, this allows for other libraries to implement
the handling of time themselves. This could be nice for things like wasm
where we can't use the standard library to get the time, but we sitll
have access to the time through other libraries.
@benthecarman
Copy link
Contributor Author

At least for the scorer, there's also an alternative way to go - give it timer ticks like we do ChannelManager/PeerManager. The nice thing about going that way is it actually reduces the amount of work we have to do during scoring, and can make routing faster as we no longer have to check + decay bounds during scoring.

Yeah I still think where ever this is used, it makes sense to give no-std users the option of having time.

@TheBlueMatt
Copy link
Collaborator

In general, yea, so let's see, Time is currently used in:

  • payment retry describing - Retry::Timeout, but that's independently gated on std, so we'd have to bound the ChannelManager or do this via timer ticks,
  • In the scorer, which I'd personally like to see get replaced with timer ticks for a performance gain.

and I think that's it? So we should consider how to provide Retry::Timeout functionality for no-std, probably, but I think that's best done by moving that whole thing to timer ticks. Then if we move scoring to timer ticks as well to avoid decaying during routing we can drop time entirely rather than expose it :)

@benthecarman
Copy link
Contributor Author

Yeah that sounds better, if you guys can get that done before next release that'd be great, otherwise I'd be in favor of merging this in the meantime.

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.

3 participants