[bug]: fn/v2: test flake in TestPropForEachConcOutperformsMapWhenExpensive

[ellemouton]

--- FAIL: TestPropForEachConcOutperformsMapWhenExpensive (0.46s)
    slice_test.go:341: #66: failed on input -485591796543117135, []int{2699404122135167197, -104612[95](https://github.com/lightningnetwork/lnd/actions/runs/13669597198/job/38217230568?pr=9577#step:9:96)91028623399}
FAIL
FAIL	github.com/lightningnetwork/lnd/fn/v2	1.700s
FAIL

// TestPropForEachConcOutperformsMapWhenExpensive ensures the property that
// ForEachConc will beat Map in a race in circumstances where the computation in
// the argument closure is expensive.

So either the test is flaky, or the above property does not actually hold true.

Appeared in this build

[Abdulkbk]

I am reviewing this issue and will share my findings shortly.

[Abdulkbk]

The test TestPropForEachConcOutperformsMapWhenExpensive compares the speed of Map and ForEachConc functions when executing an expensive operation on a slice. In this case, the expensive operation is time.Sleep(time.Millisecond).

Since the Map function operates sequentially, we expect ForEachConc to always complete first because it runs concurrently.

However ForEachConc has some overheads including creating and managing goroutines, semaphore acquisition & release, and waitgroup operations (which I think are non-deterministic).

This becomes a problem, especially when the size of a slice is small. Although we have the following check for cases like that in the test:

if len(s) < 2 {

// Intuitively we don't expect the extra overhead of

// ForEachConc to justify itself for list sizes of 1 or

// 0.

return true

}

But from what I observed after running the test several times, the only failure instances occur when the size of the slice is 2. This means considering the overheads associated with ForEachConc, the function does not always justify itself when the slice size is 2. When the slice size is 3 or more the concurrency advantage is more pronounced.

(Another potential problem could arise when there aren't enough cores in the test environment. For instance, in a scenario where ForEachConc can only spin one goroutine for each element in the slice, one at a time. It would execute them resembling a sequential run. However I believe this isn't the situation we're dealing with here.)

I can think of a few ways to resolve this issue. One option is to update the early return check to accommodate slices of size 2.

if len(s) < 3 {...}

We could also consider making the "expensive" operation more noticeable, for example, we can time.Sleep(n * time.Millisecond), where n is a number, instead of time.Sleep(time.Millisecond) but this slows the test.

Perhaps a better solution would be to measure the execution time of Map and ForEachConc separately in a deterministic way, and then assert that ForEachConc is faster based on those results??.

[yyforyongyu]

Perhaps a better solution would be to measure the execution time of Map and ForEachConc separately in a deterministic way, and then assert that ForEachConc is faster based on those results??.

Yeah I think this approach is better. I also find it weird that we use a test to assert one method performs better than the other - we usually just benchmark them and print the results instead of assertion. In addition, I'm in favor of just removing ForEachConc - I don't think it's used anywhere, and it already creates headaches for us.

[KapilSareen]

Hey @yyforyongyu, what do you think finally about this issue then? Does this need to fixed?

I also find it weird that we use a test to assert one method performs better than the other - we usually just benchmark them and print the results instead of assertion

[Abdulkbk]

Yeah I think this approach is better. I also find it weird that we use a test to assert one method performs better than the other - we usually just benchmark them and print the results instead of assertion. In addition, I'm in favor of just removing ForEachConc - I don't think it's used anywhere, and it already creates headaches for us.

Sounds good! I will investigate how benchmarking is done in other places and see how it will be applicable here as well. I would also like to hear others' opinions on the possibility of removing the ForEachConc func if we decide to pursue that route.