Skip to content

Conversation

@chrisseaton
Copy link
Member

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we should allocate this from a global counter instead? Really make sure it is unique.

Copy link
Member

Choose a reason for hiding this comment

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

Could we use SecureRandom.uuid? Would that provide sufficient uniqueness? http://www.ruby-doc.org/stdlib-2.1.1/libdoc/securerandom/rdoc/SecureRandom.html#method-c-uuid

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... that's still just probabilistic. So all we could say to users is this code will probably work. I'll do it properly and use a counter. People probably aren't creating these in an inner loop anyway.

I'm more worried about the space leak.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it could be a good idea, but what about an "upside down" implementation?
At the moment threadlocals are implemented in MRI using thread storage, but we can reverse it keeping an internal map with threads as key.

This we'll avoid every memory leak or uniqueness; I've just launched a very basic benchmark (Thread.current.thread_variable vs a map guarded by a lock) and this option seems only slightly slower with respect to the current one.

If you think this is a good idea to explore, we could try to build better benchmark

Copy link
Member Author

Choose a reason for hiding this comment

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

We should take a look at how MRI implements Thread.current and thread locals internally. It's likely that's not using any shared hash at all, so if we have our own global hash with a local around, surely that won't scale very well.

I'll commit using this approach since with a global counter it will work, and the counter shouldn't really be a problem unless we're allocating ThreadLocalVar in an inner loop, and then we can go back and look at it empirically. It's not much code to change if we find a better approach later.

Copy link
Member

Choose a reason for hiding this comment

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

I'll be honest, I don't know enough about the MRI internals to have any idea how that's implemented. I'm curious so I'm going to take a look and see if I can figure it out. I trust your judgement on this.

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.

4 participants