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

Swift 5: resolve Linux crash with concurrent access to socketHandlers #284

Merged
merged 10 commits into from
Jan 8, 2019

Conversation

djones6
Copy link
Contributor

@djones6 djones6 commented Dec 17, 2018

Description

Ensures that modifications to the IncomingSocketManager.socketHandlers dictionary are performed with exclusive access, using a DispatchQueue to access the dictionary in handle(), process() and removeIdleSockets().

Also adds Travis for the GCD codepath on Linux.

Motivation and Context

With Swift 5, the Kitura test suite crashes intermittently (but frequently) while accessing the socketHandlers dictionary. I believe this is due to concurrent access but I haven't managed to get a clear picture from lldb so far. Adding locking around accesses to the dictionary avoids the problem.

How Has This Been Tested?

Running under stress using a simple Kitura 'hello world' benchmark results in a crash almost immediately with Swift 5. I've confirmed that the process does not crash after applying this fix.

Checklist:

  • I have submitted a CLA form
  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

@djones6
Copy link
Contributor Author

djones6 commented Dec 19, 2018

The use of NSLock works, but adds a ~7% performance penalty for epoll (plaintext 'Hello World' benchmark running on 16.04 / 4 hw threads):

               | Throughput (req/s)      | CPU (%) | Mem (kb)     | Latency (ms)                   | good
Implementation | Average    | Max        | Average | Avg peak RSS | Average  | 99%      | Max      | iters
---------------|------------|------------|---------|--------------|----------|----------|----------|-------
          base |    62291.0 |    64219.6 |    97.7 |        25923 |      2.0 |      4.3 |     17.1 |    10
       locking |    58175.8 |    59268.4 |    95.9 |        26271 |      3.1 |     14.4 |    200.9 |    10
      base_gcd |    42506.2 |    44061.1 |    82.4 |        23991 |      3.0 |      4.0 |     19.9 |    10
      lock_gcd |    42224.6 |    43255.4 |    82.4 |        26551 |      3.0 |      4.0 |     19.5 |    10

@billabt
Copy link

billabt commented Dec 19, 2018

@djones6. Try using a dispatch semaphore, that seems to be more efficient. I used it in an app I was working on before I retired, replacing an NSLock, and it seemed noticeably faster. Probably worth a shot.

@djones6 djones6 force-pushed the lockSocketHandlers branch from 53efd57 to 9a0d1da Compare January 4, 2019 10:01
@djones6
Copy link
Contributor Author

djones6 commented Jan 8, 2019

I've become convinced that changes to Dictionary in Swift 5 have exposed a problem with our code, since it accesses/modifies the socketHandlers dictionary on multiple threads. Though I have yet to nail down specifically what has changed, I gather from previous Swift 5 fixes relating to Dictionaries and ordering that there are some substantial changes under the covers.

Attempting to separate the dictionary into separate parts didn't seem to help, and neither did reducing the number of epoll threads to 1. But locking definitely prevents the crash. This leads me to believe that the problem is concurrency related, but not in the way I thought.

So I returned to evaluating various methods of locking to find the one with the lowest overhead. Here is a comparison of four different methods:

  • base: no locking (only runs with Swift 4.2.1)
  • nslock: using NSLock
  • semaphore: using a DispatchSemaphore
  • rwlock: using pthread_rwlock, using read / write locks
  • gcd: using a concurrent DispatchQueue and dispatching synchronously, with a .barrier to enforce exclusive access during modification
               | Throughput (req/s)      | CPU (%) | Mem (kb)     | Latency (ms)                   | good
Implementation | Average    | Max        | Average | Avg peak RSS | Average  | 99%      | Max      | iters
---------------|------------|------------|---------|--------------|----------|----------|----------|-------
       base421 |    63448.3 |    66149.0 |    97.4 |        26174 |      2.0 |      4.0 |     21.2 |    10
     nslock421 |    58181.2 |    61006.5 |    95.4 |        26146 |      3.1 |     15.8 |    202.5 |    10
  semaphore421 |    58684.4 |    62353.3 |    95.7 |        26152 |      3.1 |     15.5 |    201.8 |    10
     rwlock421 |    64073.5 |    66660.2 |    97.4 |        26120 |      2.0 |      4.4 |    199.0 |    10
        gcd421 |    63671.2 |    65527.9 |    97.2 |        26165 |      2.0 |      3.8 |    202.0 |    10
		
Swift 5:
        nslock |    53019.2 |    56505.5 |    98.5 |        29191 |      2.5 |     14.0 |    201.7 |    10
     semaphore |    53832.9 |    56190.0 |    98.3 |        29150 |      2.5 |     14.0 |    200.6 |    10
        rwlock |    53266.2 |    54139.6 |    99.0 |        29148 |      2.3 |      2.7 |    199.9 |    10
           gcd |    53529.5 |    54392.8 |    99.1 |        29218 |      2.3 |      3.0 |    202.0 |    10

The read/write style of locking is much faster, almost indistinguishable from baseline. We could choose either of these methods, and I'm inclined to go with the Dispatch approach as although pthread_rwlock is perhaps a fraction faster, it's less readable and there's more scope for error.

One other thing that came out of this experiment is that Swift 5 is way slower. This was a surprise, as with the work done to utf8 handling in 5, I expected to see gains here. This will need investigating separately.

@djones6 djones6 requested a review from ianpartridge January 8, 2019 11:24
@djones6 djones6 merged commit c88223a into master Jan 8, 2019
@djones6 djones6 deleted the lockSocketHandlers branch January 8, 2019 14:04
djones6 referenced this pull request in Kitura/LoggerAPI May 23, 2019
Add comments on public API

Use isSwiftLogging() so that mapping between loggers is not duplicated
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.

3 participants