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

Refactor traverseValidationA' (perf). #110

Merged
merged 1 commit into from
Feb 25, 2021

Conversation

isaacabraham
Copy link
Contributor

@isaacabraham isaacabraham commented Feb 25, 2021

We noticed that when working with moderately large collections (10k items or more), that under certain circumstances traverseValidationA' can become very slow. It turns out this only generally occurs if all items in the collection are Ok _. The offending expression is this:

   | Ok ys, Ok y ->
     traverseValidationA' (Ok (ys @ [y])) f xs

This creates a new list for every item in the list and then joins it to another one. This PR replaces that by using the :: (cons) operator to join at the front, and then at the end of the recursive loop calls List.rev on it.

| Ok items, [] ->
  Ok (List.rev items)
...

    | Ok ys, Ok y ->
       traverseValidationARev (Ok (y :: ys)) f xs

I've compared old and new implementations with FSCheck and they appear to behave the same before / after.

Performance with Benchmark dotnet on a collection of 1000 items (note that performance does not appear to scale linearly - try this with 100,000 Oks and you'll be waiting a while...).

Three tests - all errors, all successes, or 50/50 split randomly distributed throughout the collection.

|     Method |       run |        Mean |     Error |    StdDev | Ratio | RatioSD |
|----------- |---------- |------------:|----------:|----------:|------:|--------:|
|   Original |    Errors |    12.56 us |  0.237 us |  0.233 us |  1.00 |    0.00 |
| Refactored |    Errors |    12.85 us |  0.243 us |  0.270 us |  1.02 |    0.03 |
|            |           |             |           |           |       |         |
|   Original |       Mix |    14.22 us |  0.297 us |  0.861 us |  1.00 |    0.00 |
| Refactored |       Mix |    14.52 us |  0.236 us |  0.197 us |  1.01 |    0.08 |
|            |           |             |           |           |       |         |
|   Original | Successes | 2,887.14 us | 50.531 us | 44.794 us | 1.000 |    0.00 |
| Refactored | Successes |    14.73 us |  0.185 us |  0.164 us | 0.005 |    0.00 |

P.S. Apologies for the removal of extra EOL characters which distract from the PR - VSCode did that!

@TheAngryByrd
Copy link
Collaborator

Awesome! Thanks a ton.

@TheAngryByrd TheAngryByrd merged commit baed4e6 into demystifyfp:master Feb 25, 2021
@isaacabraham isaacabraham deleted the validation-perf branch February 25, 2021 20:27
@isaacabraham
Copy link
Contributor Author

@TheAngryByrd Something I neglected to mention earlier - there might be other instances of this elsewhere in the codebase. Doing a search for @ [ shows the following other occurrences:

src\FsToolkit.ErrorHandling\List.fs:
  12          let! ys = state
  13:         return ys @ [y]
  14        }

  26            let! y = f x
  27:           return ys @ [y]
  28          }

  55        | Ok ys, Ok y ->
  56:         traverseResultA' (Ok (ys @ [y])) f xs
  57        | Error errs, Error e ->

  70          | Ok ys, Ok y ->
  71:           return! traverseAsyncResultA' (AsyncResult.retn (ys @ [y])) f xs
  72          | Error errs, Error e ->

src\FsToolkit.ErrorHandling.JobResult\List.fs:
  13            let! y = f x
  14:           return ys @ [y]
  15          }  

  37          | Ok ys, Ok y -> 
  38:           return! traverseJobResultA' (JobResult.retn (ys @ [y])) f xs
  39          | Error errs, Error e -> 

These are probably good candidates for optimisation as they probably suffer from the same issue.

TheAngryByrd added a commit that referenced this pull request Feb 25, 2021
- Performance enhancements for traverseValidationA. Credits [@isaacabraham](https://github.com/isaacabraham) - (#110)
TheAngryByrd added a commit that referenced this pull request Feb 25, 2021
- Performance enhancements for traverseValidationA. Credits [@isaacabraham](https://github.com/isaacabraham) - (#110)
@TheAngryByrd
Copy link
Collaborator

Ok I'll try to knock them out unless you can send another PR quickly :)

TheAngryByrd added a commit that referenced this pull request Feb 26, 2021
TheAngryByrd added a commit that referenced this pull request Feb 26, 2021
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.

2 participants