[LazyFilterSequence] Preserve order of predicates

[dtorres]

The current behaviour leads to unexpected order of execution (reverse) and given a cheaper filter being applied before more expensive ones, an increase in performance cost for end users.

This fixes it by preserving the input order of the predicates

Resolves SR-11841.

[dtorres]

Don't know if there is a benchmark that can expose this issue yet, here is one just in case:

import Foundation
var array = Array(repeating: "1", count: 10000)


func benchmark(name: String, _ fn: () -> Void) {
    let start = CFAbsoluteTimeGetCurrent()
    fn()
    let end = CFAbsoluteTimeGetCurrent()
    print(name + " took: \(end-start)")
}

benchmark(name: "normal") {
    _ = array
        .filter({ _ in
            return false
        }).filter { _ in
	    Thread.sleep(forTimeInterval: 0.0001)
            return true
    }
}

benchmark(name: "lazy") {
    let seq = array.lazy
        .filter({ _ in
            return false
        }).filter { _ in
	    Thread.sleep(forTimeInterval: 0.0001)
            return true
    }
    _ = Array(seq)
}

benchmark(name: "lazy reverse order") {
    let seq = array.lazy
        .filter({ _ in
	    Thread.sleep(forTimeInterval: 0.0001)
            return true
        }).filter { _ in
            return false
    }
    _ = Array(seq)
}

Results under Swift 5.1.2

> swift test.swift
normal took: 0.0032050609588623047
lazy took: 1.3336870670318604
lazy reverse order took: 0.004425048828125

[CodaFi]

Good catch!

Could you please add a regression test somewhere in test/stdlib or the validation tests to make sure this doesn’t happen again?

[dtorres]

I wish there was a way to add a test to the stdlib without having to checkout and compile the whole language.

Is there?

[owenv]

You may be able to follow the instructions here to build only the stdlib using a downloaded tool chain: https://swift.org/quickstart-contribution/#build-swift-stdlib-against-a-downloadable-toolchain

[dtorres]

A little update:
I'm blocked as I can't get the stdlib to build against the toolchain I downloaded. I don't know when (or if) I'll be able to get it running in my environment in a reasonable time.

This is the diff of the regression test

diff --git a/test/stdlib/Filter.swift b/test/stdlib/Filter.swift
index acadd48a53..fe1266d0cc 100644
--- a/test/stdlib/Filter.swift
+++ b/test/stdlib/Filter.swift
@@ -73,4 +73,26 @@ FilterTests.test("single-count") {
   expectEqual(30, count)
 }
 
+FilterTests.test("filter order") {
+    let array = [1]
+
+    let lazyFilter = array.lazy
+        .filter({ _ in
+            return false
+        }).filter { _ in
+            //assert failure
+            return true
+        }
+    _ = Array(lazyFilter)
+
+    _ = array
+        .filter({ _ in
+            return false
+        })
+        .filter { _ in
+            //assert failure
+            return true
+        }
+}
+
 runAllTests()

If someone (who can verify the test compiles and fails as it should in the current master) can get it in a different PR, would be great so we can get this fix in.
Thanks

[natecook1000]

@swift-ci Please smoke test

[natecook1000]

Thanks, @dtorres — I've added your test in a separate commit. Merges are locked until Dec 2 (and we might need to discuss the change in behavior), so we'll need to wait a bit even once tests pass.

[natecook1000]

@swift-ci Please smoke test

Helps if I call the right method! 🤦‍♂

[CodaFi]

@swift-ci please smoke test

[CodaFi]

Awesome. Thanks!

⛵️