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

Async / buffering wrapper: Improve performance when blocking on NetCore #3416

Merged

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented May 22, 2019

ConcurrentRequestQueue - Improved DequeueBatch performance when blocking

Avoid activating throttle logic, when timer-thread is able to keep up. The throttle logic also affects the timer-thread, so only perform throttle-logic when timer-thread is falling behind.

And unit test for deadlock issue

@codecov-io
Copy link

codecov-io commented May 23, 2019

Codecov Report

Merging #3416 into release/4.6.4 will decrease coverage by <1%.
The diff coverage is 89%.

@@              Coverage Diff               @@
##           release/4.6.4   #3416    +/-   ##
==============================================
- Coverage             80%     80%   -<1%     
==============================================
  Files                358     358            
  Lines              28373   28378     +5     
  Branches            3785    3784     -1     
==============================================
- Hits               22750   22740    -10     
- Misses              4537    4543     +6     
- Partials            1086    1095     +9

@snakefoot snakefoot force-pushed the NetCoreAsyncQueueDeadlock branch 3 times, most recently from 7b6a5a3 to d59e833 Compare May 23, 2019 18:41
@304NotModified 304NotModified changed the title ConcurrentRequestQueue - Improved DequeueBatch performance when blocking Async / buffering wrapper: Improve performance when blocking May 23, 2019
@304NotModified 304NotModified added this to the 4.6.4 milestone May 23, 2019
Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

👍

@snakefoot
Copy link
Contributor Author

Apparently hard to make a stable unit-test with multi-threading on the build-engines. One more try.

@snakefoot snakefoot force-pushed the NetCoreAsyncQueueDeadlock branch 6 times, most recently from 48d12d4 to 721177a Compare May 23, 2019 20:43
@snakefoot
Copy link
Contributor Author

Hrmm. Don't like how the NetCoreApp2 unit test have become very unstable. Need to be checked.

@snakefoot snakefoot force-pushed the NetCoreAsyncQueueDeadlock branch 3 times, most recently from 6a9b524 to 995a478 Compare May 23, 2019 21:10
@304NotModified
Copy link
Member

Yes please check. Unstable tests are really slowing down development and releases. Maybe we should partly mock the unit test.

@304NotModified
Copy link
Member

It's probably now more an integration test, like most "unit tests" in NLog.

@snakefoot snakefoot force-pushed the NetCoreAsyncQueueDeadlock branch 2 times, most recently from d1d93b3 to 70062ce Compare May 23, 2019 22:13
@snakefoot
Copy link
Contributor Author

Actually think it is a real bug, that has been revealed by the optimization of the dequeue-operation for the timer-thread. But right now the appveyor-build-queue has become very long, so it will have to look at it another day.

@304NotModified
Copy link
Member

304NotModified commented May 23, 2019

Maybe it's easier to run only on your appveyor account for this? Then you could stop/restart it yourself

See https://github.com/NLog/NLog/blob/dev/howto-build-your-fork.md

@snakefoot snakefoot force-pushed the NetCoreAsyncQueueDeadlock branch 4 times, most recently from d23a6fe to d9f2cc4 Compare May 24, 2019 05:15
@snakefoot
Copy link
Contributor Author

Think I have found the bug. After we have changed so blocking decrement/increments, then it means that timer-thread no longer automatically reschedules itself because it will not "see" producers by non-zero _count.

It is important now that the producers that wake up will remember to start the consumer-timer-thread, if needed.

Two producers that are blocked will get into TrySpinWaitForLowerCount() and it only performs Interlocked.Read. This means two producers will return with currentCount == 2. But one of them should have returned with currentCount = 1.

This causes both producers to say that they don't need to start the consumer-timer-thread

@304NotModified
Copy link
Member

makes sense!

Thanks for figuring out and fixing this!

@304NotModified 304NotModified merged commit eaee9e0 into NLog:release/4.6.4 May 24, 2019
@snakefoot snakefoot changed the title Async / buffering wrapper: Improve performance when blocking Async / buffering wrapper: Improve performance when blocking on NetCore May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants