Skip to content

Updates to ThreadLocalVar #387

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

Merged
merged 2 commits into from
Jul 31, 2015
Merged

Updates to ThreadLocalVar #387

merged 2 commits into from
Jul 31, 2015

Conversation

jdantonio
Copy link
Member

This PR included two updates, both of which warrant discussion.

  1. Change implementation from fiber-local to thread-local as suggested by @schmurfy here
  2. Returns the JRuby-specific implementation that was removed in f073cac.

In my benchmark tests the JRuby version performs significantly worse. The advantage, however, is that it leverages the JVM directly rather than using our Ruby implementation. In our experience, JRuby interop is significantly slower than pure-Java wrappers. Thus we've begun creating pure-Java implementations for some abstraction. It's possible that a pure-Java wrapper--which could be added at any time--will perform significantly better.

My preference is to use the JRuby version since it leveraged the JVM directly, but my feelings aren't strong. I could be convinced otherwise. If memory serves, @pitr-ch echoed this sentiment, but I can't find the comment. I may be wrong on that.

// @alexdowad

@alexdowad
Copy link
Contributor

In benchmarking, I found that a significant amount of performance was lost from calling #value= -> #bind -> #set, rather than just putting the class-specific storage implementation in #value=. You may want to try and see how much of a difference it makes. Personally, I think a noticeable boost in performance is worth duplicating perhaps 10 lines of code.

@jdantonio
Copy link
Member Author

That makes sense. Especially after looking at the code again. Initially I copied the code from prior to your new implementation. But then I optimized away those function calls in the Ruby version. The base class methods aren't really necessary any more.

I'll make that update today.

@jdantonio jdantonio force-pushed the more-thread-local-var branch from 19d75be to 5023a0c Compare July 30, 2015 16:14
@jdantonio jdantonio added this to the 0.9.1 Release milestone Jul 30, 2015
@jdantonio jdantonio added the enhancement Adding features, adding tests, improving documentation. label Jul 30, 2015
@jdantonio jdantonio self-assigned this Jul 30, 2015
@jdantonio
Copy link
Member Author

Updated with suggestion from @alexdowad.

@jdantonio jdantonio force-pushed the more-thread-local-var branch from 5023a0c to 8af1166 Compare July 30, 2015 16:26
jdantonio added a commit that referenced this pull request Jul 31, 2015
@jdantonio jdantonio merged commit 0bd6a48 into master Jul 31, 2015
@jdantonio jdantonio deleted the more-thread-local-var branch July 31, 2015 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants