Skip to content
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

Rate control 273 #483

Merged
merged 4 commits into from
Oct 22, 2018
Merged

Rate control 273 #483

merged 4 commits into from
Oct 22, 2018

Conversation

Gilthoniel
Copy link
Contributor

This PR improves caching of Tree and Roster in the overlay implementation by adding an expiration to cached entries.

Fixes #273

@Gilthoniel Gilthoniel added this to WIP in Cothority via automation Oct 19, 2018
@Gilthoniel Gilthoniel moved this from WIP to Ready4Merge in Cothority Oct 19, 2018
cache.go Outdated
// defined at the cache creation
// The implementation is robust against concurrent call
type cacheTTL struct {
entries map[interface{}]*cacheTTLEntry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically it's better to use value types instead of pointer types like interface{} as keys. that's because two different pointers might have the same value which will create duplicates in the cache. The functions where Set/Get is called like RegisterRoster and RegisterTree often take parameters that originate from the network. So, if Set gets called, then Get gets called somewhere else on something that has the same value but a different address, then the cache will miss.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think RosterID and TreeID are, at the end of the day, array types, so maybe setting the key to [32]byte will work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, my bad. I did not notice that cacheTTL is like an abstract class. the entries are concrete type when implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so as discussed and to reduce confusion I changed the key to be of UUID type (instead of the raw type to avoid breaking changes from the package)

overlay.go Show resolved Hide resolved
@kc1212
Copy link
Contributor

kc1212 commented Oct 19, 2018

Is it possible to use your new cache structure for treeNodeCache too?

@Gilthoniel
Copy link
Contributor Author

Yes totally, I'm going to move the logic

@jeffallen jeffallen merged commit dc03807 into master Oct 22, 2018
Cothority automation moved this from Ready4Merge to Closed Oct 22, 2018
@jeffallen jeffallen deleted the rate_control_273 branch August 18, 2020 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Cothority
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants