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

Remove dependency on concurrent-ruby #63

Closed
wants to merge 2 commits into from
Closed

Conversation

@jmettraux
Copy link

jmettraux commented Jan 3, 2017

Concurrent-ruby weighs 129KB (234KB for the Java flavour). Tzinfo only uses its Concurrent::Map.

This commit implements a tiny TZInfo::ConcurrentMap for @@countries and @@loaded_zones

Makes TZInfo lean.

Many thanks for TZInfo.

Concurrent-ruby weighs 129KB (234KB for the Java flavour). Tzinfo only uses its Concurrent::Map.

This commit implements a tiny TZInfo::ConcurrentMap for @@countries and @@loaded_zones

Makes TZInfo lean.
@philr
Copy link
Member

philr commented Jan 3, 2017

Thanks for submitting this pull request. However, I don't think I'm going to merge it.

TZInfo only depends on the part of concurrent-ruby that it uses (require 'concurrent/map'). This reduces the memory required at runtime compared to loading the full library. After loading TZInfo, Concurrent.constants returns just [:NULL, :Map, :Collection, :Synchronization, :Utility] (compared to the 87 constants defined after running require 'concurrent').

With your TZInfo::ConcurrentMap implementation, readers will block other readers. As I understand it, the MRI and JRuby implementations of Concurrent::Map allow simultaneous concurrent reads. The places where Concurrent::Map is used in TZInfo are likely to involve significantly more reads than writes, so allowing concurrent access is useful.

@jmettraux
Copy link
Author

jmettraux commented Jan 3, 2017

Thanks a lot. Now I just wish concurrent/map (and all its concurrent friends) were part of Ruby.

Happy new year!

@jmettraux
Copy link
Author

jmettraux commented Jan 3, 2017

Well, on the other, the mutex synchronisation on read could be removed, so that only writes do block.

@philr
Copy link
Member

philr commented Jan 3, 2017

Synchronizing just the write operations is not safe on all Ruby implementations and may not be safe for MRI in the future. See #45.

@jmettraux
Copy link
Author

jmettraux commented Jan 3, 2017

Please close then. Thanks.

@philr philr closed this Jan 3, 2017
@jmettraux
Copy link
Author

jmettraux commented Jan 3, 2017

@philr
Copy link
Member

philr commented Jan 3, 2017

Those comments refer to there being no locking around finding there is no cached instance of a particular Country or Timezone and then creating and storing a new instance. This means that it is possible that multiple instances of the equivalent Country or Timezone could be concurrently created, but only one would ultimately be retained.

A plain Hash is not sufficient because it cannot be safely updated concurrently by multiple threads without external locking.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.