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

203 - Removing lock in Scope to improve performance #322

Merged
merged 10 commits into from
Oct 11, 2020
Merged

203 - Removing lock in Scope to improve performance #322

merged 10 commits into from
Oct 11, 2020

Conversation

dadhi
Copy link
Owner

@dadhi dadhi commented Sep 19, 2020

No description provided.

@dadhi dadhi changed the title Fix for the issue - removing lock in Scope to improve performance 203 - Removing lock in Scope to improve performance Sep 19, 2020
@@ -3806,6 +3806,28 @@ public static ImMapEntry<V> GetEntryOrDefault<V>(this ImMap<V> map, int key)
return entry != null && entry.Key == key ? entry : null;
}

/// <summary>Looks for the sure present entry - in cases when we know for certain that the map contains the entry</summary>
[MethodImpl((MethodImplOptions)256)]
public static ImMapEntry<V> GetSurePresentEntry<V>(this ImMap<V> map, int key)
Copy link
Contributor

Choose a reason for hiding this comment

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

cool, if we know something is presented, than optimize!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, my luck that I am in full control of the data structure.

| Method | Mean | Error | StdDev | Ratio | RatioSD | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------- |---------:|----------:|----------:|------:|--------:|-------:|------:|------:|----------:|
| MsDI | 4.215 us | 0.0811 us | 0.0901 us | 1.00 | 0.00 | 1.0605 | - | - | 4.35 KB |
| DryIoc | 1.628 us | 0.0307 us | 0.0272 us | 0.39 | 0.01 | 0.7248 | - | - | 2.96 KB |
Copy link
Contributor

Choose a reason for hiding this comment

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

no so great :(

Copy link
Owner Author

@dadhi dadhi Sep 19, 2020

Choose a reason for hiding this comment

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

No, this is fine :) This is the comparison of just commenting lock vs complete solution, the original result for current v4.4.1 (with the lock) is higher in the same file.

The prev results:

|             DryIoc | 1.868 us | 0.0226 us | 0.0200 us |  0.44 |    0.01 | 0.7248 |     - |     - |   2.96 KB |
| DryIoc_MsDIAdapter | 2.651 us | 0.0404 us | 0.0338 us |  0.63 |    0.02 | 0.7286 |     - |     - |   2.98 KB |

@@ -46,6 +46,9 @@ THE SOFTWARE.
#if !PCL && !NET35 && !NET40 && !NET403
#define SUPPORTS_DELEGATE_METHOD
#endif
#if !NET35 && !PCL
Copy link
Contributor

Choose a reason for hiding this comment

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

why not to drop net35 and pcl?

Copy link
Owner Author

@dadhi dadhi Sep 19, 2020

Choose a reason for hiding this comment

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

Dropping them in V5.
There is v5-dev branch where I did that allready. So the current fallback to lock will be removed in v5.

#if SUPPORTS_SPIN_WAIT
var spinWait = new SpinWait();
while (otherItemRef.Value == NoItem)
spinWait.SpinOnce();
Copy link
Contributor

Choose a reason for hiding this comment

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

what if createValue failed? i mean there were 2 calls. first failed, than second will spin until third call will be done?

Copy link
Owner Author

Choose a reason for hiding this comment

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

If it failed then the exception will stop everything anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

one thread said it will create value and factory delegate failed, the other thread was spinning, how that other will pass? lock, if locked delegate failed, allowed others to proceed.

Copy link
Owner Author

@dadhi dadhi Sep 19, 2020

Choose a reason for hiding this comment

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

The app will crash with all the threads, so it does not matter if one was spinned or not.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I am not sure what do you mean?

You mean the spinned thread will leak outside of the app? How?

Copy link
Contributor

Choose a reason for hiding this comment

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

what if do try catch of one thread, any chance second thread will loop forever?

while (otherItemRef.Value == NoItem)
spinWait.SpinOnce();
#else
lock (itemRef)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about details of lock, never read it, but if lock is somethings have some averaging complex heuristics to spin until lock and this heuristics adds extra layers, but safes from CPU of spins in case of long locks, so it is good and safe on average. so removing it and replacing with custom heuristics in that spin is better in our case or write custom lock with different may be real time changed heuristics, but give results of benchmarks seems that is of not so big help? and more risks.

Copy link
Owner Author

Choose a reason for hiding this comment

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

See my comment for benchmark, it is huge 10+ wing in the already highly optimized code.

And again, I almost never expect for the parallel code accessing the scope, so the majority of cases will never spin.
Also SpinWait has an adaptive implementation as well which will start yielding and thread sleeping after cirtain amount of iterations.

Copy link
Contributor

@dzmitry-lahoda dzmitry-lahoda Sep 19, 2020

Choose a reason for hiding this comment

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

while (otherItemRef.Value == NoItem)
                        spinWait.SpinOnce();

Copy link
Contributor

Choose a reason for hiding this comment

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

what is condition for while to stop? is any chance for infinite loop?

Copy link
Owner Author

@dadhi dadhi Sep 19, 2020

Choose a reason for hiding this comment

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

while (otherItemRef.Value == NoItem)

The condition will be met when createValue returns. If it does not return by hanging or throwing then we have the others problems to solve anyway.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I may add the timeout here but it will just add more non-determinizm and it kind of hard to decide on the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

so yes, it was not about timeout, but about otherItemRef.Value changes it state to other to allow spin to pass, so all is ok.

Copy link
Owner Author

Choose a reason for hiding this comment

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

lock (itemRef)

This is a copy-paste bug, it should be lock around otherItemRef.

#if SUPPORTS_SPIN_WAIT
var spinWait = new SpinWait();
while (otherItemRef.Value == NoItem)
spinWait.SpinOnce();
Copy link
Contributor

Choose a reason for hiding this comment

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

one thread said it will create value and factory delegate failed, the other thread was spinning, how that other will pass? lock, if locked delegate failed, allowed others to proceed.

@dzmitry-lahoda
Copy link
Contributor

other possible check - if other containers do the same.

@dadhi
Copy link
Owner Author

dadhi commented Sep 19, 2020

what if do try catch of one thread, any chance second thread will loop forever?

There is no possibility for it.
I am controlling how createValue is called and won't do the try-catch.
Assuming someone put the try-catch inside the createValue, then it still need to return something (including null) or re-throw. If it returns, then whatever value it is, it won't be NoItem. If it throws - app will crash, means we are good too.

The only possible hack is returning the NoItem by createValue, but this thing is internal and if you do the reflection hacking - then you deserve the problems :)

@dadhi dadhi linked an issue Sep 20, 2020 that may be closed by this pull request
@dadhi
Copy link
Owner Author

dadhi commented Oct 11, 2020

I have replaced the locks everywhere and found one of the recursion issues in the process. It is now fixed

@dadhi dadhi merged commit de9f0a7 into master Oct 11, 2020
@dadhi dadhi deleted the issue_203 branch April 2, 2021 06:33
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.

[Perf] Avoid locking of service creation in scope if possible
2 participants