Use Optional's built-in map

[chriseidhof]

I was reading through the source and saw this usage of force-casting an optional. I think the code reads cleaner using the built-in map on Optional. I can also imagine an if let would be a good alternative, in case using the built-in map has a performance overhead (not sure if this is the case for generic/rethrows functions, or if that gets compiled away).

[gribozavr]

@nadavrot This is a readability win, and should keep performance unchanged.

[gribozavr]

I'm running tests and will merge if tests pass.

[chriseidhof]

That makes a lot of sense. How do you want the tests to be delivered? I have an XCTestCase here, it's not very high-tech. Just for future reference, how should we do this?

I ran the test case, and the numbers are very close to each other. Sometimes the one is slightly faster, sometimes the other. I'm not a performance expert at all, but I'm pretty sure this is because of other processes on my computer.

[gribozavr]

@nadavrot The SIL looks almost exactly the same to me, except for extra arguments in one of the basic blocks, and an extra retain/release for the closure. I think your team should look at this. There is no reason for the closure to be subject to any retains/releases here, I think.

public struct LazyMapGeneratorTest<
  Base : GeneratorType, Element
> : GeneratorType, SequenceType {
  public mutating func next() -> Element? {
    return _base.next().map(_transform)
  }

  public mutating func next_old() -> Element? {
    let x = _base.next()
    if x != nil {
      return _transform(x!)
    }
    return nil
  }

  public var base: Base { return _base }
  internal var _base: Base
  internal var _transform: (Base.Element)->Element
}

public func callNextNew(xs: LazyMapGeneratorTest<Array<Int>.Generator, Int>) -> Int? {
  var xs = xs
  return xs.next()
}

public func callNextOld(xs: LazyMapGeneratorTest<Array<Int>.Generator, Int>) -> Int? {
  var xs = xs
  return xs.next_old()
}

Old:

bb39:                                             // Preds: bb1 bb19
  %348 = load %10#1 : $*Optional<Int>             // user: %354
  dealloc_stack %10#0 : $*@local_storage Optional<Int> // id: %349
  strong_release %7 : $Builtin.BridgeObject       // id: %350
  strong_release %3 : $@callee_owned (@out Int, @in Int) -> () // id: %351
  strong_release %7 : $Builtin.BridgeObject       // id: %352
  strong_release %3 : $@callee_owned (@out Int, @in Int) -> () // id: %353
  return %348 : $Optional<Int>                    // id: %354

New:

bb39(%354 : $Optional<Int>, %355 : $@callee_owned (@out Int, @in Int) -> @error ErrorType): // Preds: bb1 bb19
  strong_release %355 : $@callee_owned (@out Int, @in Int) -> @error ErrorType // id: %356
  strong_release %7 : $Builtin.BridgeObject       // id: %357
  strong_release %3 : $@callee_owned (@out Int, @in Int) -> () // id: %358
  strong_release %7 : $Builtin.BridgeObject       // id: %359
  strong_release %3 : $@callee_owned (@out Int, @in Int) -> () // id: %360
  return %354 : $Optional<Int>                    // id: %361

The functional tests passed, by the way (build-script -RT).

[nadavrot]

@aschwaighofer @gottesmm Do you have an opinion here?

[aschwaighofer]

We don't get rid of a use of the error typed version of the closure after this change: $@callee_owned (@out Int, @in Int) -> @error ErrorType.

I think we should look at why this is before accepting this change as the extra refcount operation can negatively affect performance.

[aschwaighofer]

I looked at the code example and the generated code looks better for the map case. @gribozavr probably used a configuration which builds the stdlib with asserts.

I am good with this going in.

[nadavrot]

Thank you!