-
Notifications
You must be signed in to change notification settings - Fork 569
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
Removing statics from codebase #117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FasterThreadLocal<T>.InitializeThread
isn't thread-safe. It is being called in LightEpoch.Acquire
and can lead to concurrency issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how the current implementation of FastThreadLocal<T>
is instance specific? I am confused because the values array is thread static which I suppose means that two objects of FastThreadLocal<int>
access the same values array if done from the same thread.
Fixed as follows: if (values == null)
{
Interlocked.CompareExchange(ref values, new T[kMaxInstances], null);
}
FastThreadLocal has a thread-static array, i.e., each thread sees its own copy of the array. Each instance of the object (regardless of threads) will correspond to a unique entry offset ( |
Why is this necessary for a thread static variable? Won't each thread sees a different copy of "values", making the Interlocked operation unnecessary? |
Correct, it's not needed. My original code is fine, I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕐
…l.values for threads that haven't initialized values yet
commit 2ba964526c96c5c6852d1b702348a5bc9fa94dcb Merge: f2b2c1f 765c083 Author: Badrish Chandramouli <badrishc@microsoft.com> Date: Thu Apr 4 10:54:51 2019 -0700 Merge branch 'nostatics' of https://github.com/Microsoft/FASTER into readobjfix commit f2b2c1f84b0bac97da222de4f3c929743742ecf2 Author: Badrish Chandramouli <badrishc@microsoft.com> Date: Thu Apr 4 10:54:09 2019 -0700 Fixes to addresses, cleanup object read logic commit 765c083 Author: Peter Freiling <peterfr@microsoft.com> Date: Thu Apr 4 10:01:26 2019 -0700 Fixing issue where LightEpoch.IsProtected dereferences FastThreadLocal.values for threads that haven't initialized values yet commit b78aee3 Author: Peter Freiling <peterfr@microsoft.com> Date: Wed Apr 3 20:43:28 2019 -0700 Removing extra buffer copy commit cd0da5ec37469486e0b6cd978c2088a5ee6bb598 Author: Badrish Chandramouli <badrishc@microsoft.com> Date: Wed Apr 3 19:59:53 2019 -0700 Updates commit 8d2ceeb6afcdaf2dacb98c0fa905a66bba7f3ae9 Author: Badrish Chandramouli <badrishc@microsoft.com> Date: Wed Apr 3 19:22:39 2019 -0700 Fix for reading objects
We have used ThreadStatic until now, for storing thread-local variables. This does not work when you have multiple instances of FASTER operating at the same time. This PR fixes the problem by using instance-specific thread-local variables instead. We have implemented our own version of thread-local for this purpose, called FastThreadLocal. Incidentally, FastThreadLocal is optimized to be around 10x faster than ThreadLocal in our micro-benchmarks. There is a configurable (in code) limit of 128 instances of a particular thread-local variable type. This may be relaxed in future if the requirement comes up. With this PR, there are no shared objects between instances, except for the buffer pool which is shared static for the entire process.