Skip to content

List.fold/contains simplification#3975

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.

Sign up for GitHub

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

Merged
KevinRansom merged 1 commit into from
Nov 20, 2017
Merged

List.fold/contains simplification #3975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 9 additions & 15 deletions src/fsharp/FSharp.Core/list.fs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -212,15 +212,11 @@ namespace Microsoft.FSharp.Collections

[<CompiledName("Fold")>]
let fold<'T,'State> folder (state:'State) (list: 'T list) =
match list with
| [] -> state

@dsymedsymeNov 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This optimization for the case where the list is empty has been removed - can it please be added back in? Or lets just not do this change

@ncavencaveNov 20, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyme Thanks, I'll add it. The optimization for the empty case is still there to some extent, in the sense that the folding function is not being called in the "empty list" case. But yes, additional check in the beginning for the empty case makes a lot of sense, so the Adapt function is not called prematurely. I'll resubmit.

@dsymedsymeNov 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I integrated the revert so you can submit a new PR at your leisure :) thanks all!

| _ ->
let f = OptimizedClosures.FSharpFunc<_,_,_>.Adapt(folder)
let rec loop s xs =
match xs with
| [] -> s
| h::t -> loop (f.Invoke(s,h)) t
loop state list
let f = OptimizedClosures.FSharpFunc<_,_,_>.Adapt(folder)
let mutable acc = state
for x in list do
acc <- f.Invoke(acc, x)
acc

[<CompiledName("Pairwise")>]
let pairwise (list: 'T list) =
Expand DownExpand Up@@ -356,12 +352,10 @@ namespace Microsoft.FSharp.Collections
let exists predicate list = Microsoft.FSharp.Primitives.Basics.List.exists predicate list

[<CompiledName("Contains")>]
let inline contains value source =
let rec contains e xs1 =
match xs1 with
| [] -> false
| h1::t1 -> e = h1 || contains e t1
contains value source
let rec contains value source =

@isaacabrahamisaacabrahamNov 19, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it matter that contains is no longer marked as inline?

@ncavencaveNov 19, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isaacabraham idk, but neither are exists, find/findBack, tryFind/tryFindBack, pick/tryPick etc.

@isaacabrahamisaacabrahamNov 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm - so why was contains inlined? Just some historical artifact, or some reason.

@forkiforkiNov 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People started to inline stuff in other PRs...

@dsymedsymeNov 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the "inline" matters a lot. The inlining causes the equality check to become monomorphic (non-generic) in the common case that the callsite is monmorphic, and hence the equality check will be type-specialized

@ncavencaveNov 20, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyme I'll take it out if the inline is that important, as I'm not sure how to get around FS1118 (cannot inline recursive).
There are a few other funcs with when 'T : equality signature (except, distinct, countBy etc.), should they be inline as well?

@dsymedsymeNov 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the nested let rec is to get around the restriction on let rec inline. The whole inner let rec gets copied that way.

Was the use of a nested let rec causing problems for Fable? If so you may often be able to replace with an explicit loop

@ncavencaveNov 21, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyme Nothing to do with Fable, this optimization was just to remove the unnecessary extra object allocation and call that is generated, but I understand the inline modifier is more important here (although there are other funcs with when 'T : equality that are not inline).

@dsymedsymeNov 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the inline modifier is more important here (although there are other funcs with when 'T : equality that are not inline).

If there is performance evidence to show that it's worthwhile inlining these then we should likely do it. Normally it's easy to get perf evidence in such cases. Sometimes the inlining can't be done due to the implementation depending on other internal functions we don't want to make public

match source with
| [] -> false
| h::t -> if h = value then true else contains value t

let rec exists2aux (f:OptimizedClosures.FSharpFunc<_,_,_>) list1 list2 =
match list1,list2 with
Expand Down
2 changes: 1 addition & 1 deletion src/fsharp/FSharp.Core/list.fsi
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -102,7 +102,7 @@ namespace Microsoft.FSharp.Collections
/// <param name="source">The input list.</param>
/// <returns>True if the input list contains the specified element; false otherwise.</returns>
[<CompiledName("Contains")>]
val inline contains: value:'T -> source:'T list -> bool when 'T : equality
val contains: value:'T -> source:'T list -> bool when 'T : equality

/// <summary>Returns a list that contains no duplicate entries according to generic hash and
/// equality comparisons on the entries.
Expand Down