List.fold/contains simplification#3975
Conversation
KevinRansom
left a comment
KevinRansom
left a comment
There was a problem hiding this comment.
Nice, thanks for this.
| | [] -> false | ||
| | h1::t1 -> e = h1 || contains e t1 | ||
| contains value source | ||
| let rec contains value source = |
There was a problem hiding this comment.
Does it matter that contains is no longer marked as inline?
There was a problem hiding this comment.
@isaacabraham idk, but neither are exists, find/findBack, tryFind/tryFindBack, pick/tryPick etc.
There was a problem hiding this comment.
Hmmm - so why was contains inlined? Just some historical artifact, or some reason.
There was a problem hiding this comment.
People started to inline stuff in other PRs...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
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
|
Thank you for this. |
|
@KevinRansom Please revert this, it needs proper review. And can we in general please look at changes a bit more deeply before integrating them? For example, it removes an optimization for empty lists here.https://github.com/Microsoft/visualfsharp/pull/3975/files#diff-9c7a3b0c1ddcc1a72b924ffac75f21feL215. Perhaps we can do this, but it needs performance testing and review. It also removes the use of |
| [<CompiledName("Fold")>] | ||
| let fold<'T,'State> folder (state:'State) (list: 'T list) = | ||
| match list with | ||
| | [] -> state |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Thanks. I integrated the revert so you can submit a new PR at your leisure :) thanks all!
dsyme
left a comment
dsyme
left a comment
There was a problem hiding this comment.
As mentioned, please revert
|
@dsyme the problem is that we never know when (and if) you are going to review a PR. Is it possible to mark a PR as "must be reviewed by ....", with proper notification? (or "I'm gonna review this by the end of this week") |
|
6 participants
Generates simpler IL.