Rework of Dispatch overlay for Linux

[dgrove-oss]

The lack of Objective-C and associated bridging
when importing the libdispatch header files into Swift
caused a number of problems including
(a) missing ref count operations in Swift compiled code
(b) SR-739, dispatch types not being upcastable to dispatch_object_t
(c) SR-740, dispatch types not being upcastable to AnyObject
(d) SR-737, dispatch types being imported as COpaquePointer

This commit fixes all of these issues by injecting a complete
Swift overlay layer that wraps the native libdispatch objects
and native APIs.

The C header files are now imported into a distinct CDispatch module
that is not made available to Swift client code (which continues to
import Dispatch). Code is added to Dispatch.swift to mirror the
native CDispatch APIs up to the Swift Dispatch API and to wrap/unwrap
values as needed across that boundary.

This wrapping layer does add some minor space and time overheads,
but after extensively exploring implementing the Swift object model
within the C code of libdispatch (to avoid introducing this overhead),
I concluded that the necessary changes to libdispatch would be fairly
invasive, and therefore were not justified at this point in the
development of the Swift/Linux port of libdispatch.

[parkera]

For these and others, why introduce the type alias and the class? The class could be named "dispatch_object_t" (although it doesn't match naming conventions, it does match existing API).

[dgrove-oss]

I think either could work. I was thinking it might be useful to distinguish between the native and Swift layers of the code, but I really didn't use the capitalized class names outside the declarations. I could rework if you want.

[parkera]

I only skimmed it so far but this looks like a great start to me.

[drewcrawford]

Thanks for looking into this @dgrove-oss, it's really important stuff.

I tried to review this today, I got the following errors. Some look Swift 3 related (COpaquePointer => OpaquePointer), others I'm not sure about.

I expect I'm on the shortlist of "large Linux dispatch users" at ~400 invocations. I would be happy to fuzztest this against my codebase and see if I can turn up any regressions.

[dgrove-oss]

Thanks Drew. I'll take a look. My swift checkouts are a couple of weeks old (had fallen off tracking tip while waiting for things to stabilize). I'll have to update to latest swift levels and adjust this patch for Swift 3 changes that I hadn't been compiling against.

[parkera]

After thinking about it I'm pretty sure we should just use the existing name and not introduce the other one:

public typealias dispatch_object_t = DispatchObject // this one
public class DispatchObject {

[parkera]

Once we get this building against Swift 3 we should go ahead and merge this so we can try it out. Thanks again for your work on this Dave!

[dgrove-oss]

Updated to build against Swift 3 as of this morning. Also changed class names to eliminate typealias.

Did not squash & force push yet in case we need to revise further. Would be great to have Drew validate that he doesn't see any regressions on his test suite.

[jessesquires]

@dgrove-oss You listed SR-730 in your initial comment, but I think you meant SR-740 instead. 😄

[dgrove-oss]

squashed changes into single commit. Also fixed typo in issue number in commit comment noted by @jessesquires. As far as I can tell, this is now ready to go.

[drewcrawford]

Sorry it has taken me a few days to reply, the surprise release of Xcode 7.3 lit a few fires. They are now out.

The immediately-visible regression is

import Dispatch
var t: dispatch_once_t = 0

which produces

$ swift -Xcc -fblocks test.swift
test.swift:2:8: error: use of undeclared type 'dispatch_once_t'
var t: dispatch_once_t = 0
       ^~~~~~~~~~~~~~~

rather than builds as expected.

A second obvious regression is

import Dispatch
let s = dispatch_semaphore_create(0)
dispatch_semaphore_wait(s, DISPATCH_TIME_FOREVER)

producing

test.swift:4:25: error: value of optional type 'dispatch_semaphore_t?' not unwrapped; did you mean to use '!' or '?'?
dispatch_semaphore_wait(s, DISPATCH_TIME_FOREVER)
                        ^
                         !

instead of building as expected.

we should go ahead and merge this so we can try it out

I understand the motivation, but I think the fact that very simple regressions are being missed in manual reviews is a pretty good argument for leveraging my private testsuite to check this which can shake out problems that are hard to spot manually very easily.

That said, I do appreciate all the work that @dgrove-oss and others are doing on this and would like to see it merged as quickly as can be done without introducing breakages.

[MadCoder]

dispatch_once_t should not be used in swift, it is broken. swift has lazy initialization which covers the use case in a more integrated way.

[drewcrawford]

In that case, isn't the correct thing to do to declare with an availability modifier (deprecated / unavailable)

[MadCoder]

Yes it should be marked as unavailable in swift.

[drewcrawford]

What is the correct syntax for dispatch_once (normally takes dispatch_once_t as its first argument), or is that also deprecated?

[MadCoder]

dispatch_once can't work without dispatch_once_t (or dispatch_once_f) both should be unavailable in swift. Again, this is not a major issue as swift has lazy initialized globals and lazy closures which are a strict superset of the services dispatch_once usually provides.

(The exect reason it doesn't work is that the swift compiler can decide to relocate dispatch_once_t's (that's also why pthread_mutex doesn't work in swift if you haven't set it as an ivar of a class), and unlike a mutex, dispatch_once_t can't live in malloc()ed memory because of how the implemnetation works)

[dgrove-oss]

For dispatch_once, here is a test program that compiles and runs using the overlay API. I'm not 100% sure if it correctly handles the relocation issue Pierre is raising, but it does compile and run.

import Dispatch

var token: Int = 0
func test() {
  dispatch_once(&token) {
    print("This is printed only on the first call to test()")
  }
  print("This is printed for each call to test()")
}

for _ in 0..<4 {
  test()
}

For dispatch_semaphore_create, I intentionally made the return type be dispatch_semaphore_t? because it will return nil if it is passed a negative count. This is unlike the dispatch_queue_create, etc. which are guaranteed in the API to return non-null values. So you'd have to use ! or ? to unwrap the semaphore before using. Maybe this is overly pedantic, but I think it is a more accurate representation of what the possible returned values are.

[MadCoder]

dispatch_semaphore_create() should return a semaphore not an optional. the convention Foundation has for this is that if it is about memory missing or programming error, it shouldn't be an optional and crashing is ok.

dispatch_once will compile, but sometimes be incorrect. trivial test programs will never trip the issues that are possible and related to how Swift work (it can relocate the dispatch_once_t)

[parkera]

Let me leave this quite from the Swift Programmimg Language here:

“If a property marked with the lazy modifier is accessed by multiple threads simultaneously and the property has not yet been initialized, there is no guarantee that the property will be initialized only once.”

Excerpt From: Apple Inc. “The Swift Programming Language (Swift 2.1).” Apple Inc., 2015-10-14T07:00:00Z. iBooks.
This material may be protected by copyright.

Check out this book on the iBooks Store: https://itun.es/us/jEUH0.l

[parkera]

So yah, I think we need a way to make dispatch_once work...

[parkera]

Unless something has changed in Swift 3 that I've missed.

[dgrove-oss]

I pushed a change to make dispatch_semaphore_create's return be non-optional. Left dispatch_once unchanged for now (still available; takes a UnsafeMutablePointer as the first/predicate parameter).

[MadCoder]

@parkera: dispatch_once doesn't work for ivars and properties anyway, because it has to live in static memory. and my understanding is that globals initialized lazily are initialized once in swift like you expect.

[parkera]

Ok. I'm in favor of using the language primitives too. Hopefully that lazy limitation can be lifted in the future.

[drewcrawford]

IMO we need typealias dispatch_once_t = Int. Whether that should be unavailable or not I leave to SR-1042, but I do believe it should be declared, rather than leaving the user with a "no such type" error as if this is the first we are hearing of a 6 year old API.

In any case, I built new patch, reworked my dispatch_once usage, and got a full pass. I was able to strip out 200 lines of evil Linux hax working around SR-739, SR-740, and SR-737, which IMO are "confirmed resolved".

My huge codebase is now 1LOC away from full source compatibility with Darwin. So LGTM! 👍 Thanks to @dgrove-oss and all for putting in the work.

[dgrove-oss]

Thanks very much for testing this @drewcrawford. Great to hear it works modulo dispatch_once

[jckarter]

lazy is not a substitute for dispatch_once, but dispatch_once never worked with instance properties. For lazy global initialization, declare a global or static property, which use the equivalent of dispatch_once to evaluate the initialization expression.

[parkera]

Frankly, the compiler should reject any usage of lazy where it can't promise the correct semantics. But anyway, that's out of the scope of this particular PR.

[gparker42]

Pierre, do you have a sample program that demonstrates broken dispatch_once?

[jckarter]

We can always give lazy its specified semantics—it doesn't guarantee concurrency safety in its current form. I'm trying to clarify that it's unrelated to dispatch_once.

[dgrove-oss]

This will also resolve https://bugs.swift.org/browse/SR-1106 when it is merged.

[dgrove-oss]

@mwwa @parkera @MadCoder I'd like to move towards merging this into the experimental/foundation branch so it is available for use in our foundation/dispatch work and in downstream projects like Kitura. Are you ok with that merge going ahead on the experimental branch?

[MadCoder]

can we close this since we're moving on the topic another way?

[dgrove-oss]

Closing since this branch has merge conflicts anyways. Will open new requests with the fresh changes.