Uh oh!
There was an error while loading. Please reload this page.
Replace (nearly) all ref cells in the compiler with mutable values - #8063
Conversation
NatElkins
commented
Jan 3, 2020
Is there any performance benefit to this? Or is it just a stylistic change? |
cartermp
commented
Jan 3, 2020
Mostly stylistic, and most of the uses seem to predate |
heronbpv
commented
Jan 3, 2020
Taking the opportunity to also make a question: is there any remaining case (or cases) where usage of a ref cell is recommended over a mutable let? |
Uh oh!
There was an error while loading. Please reload this page.
@heronbpv There are, but I'd say the uses are limited. The syntax just looks kind of wonky and the dereference operator For bogstandard mutation that's local to a function or method, typically done for perf reasons, mutable values are preferred. Same goes for I'd also say that using a
In those cases, you'll need to declared something as a Additionally, tupled values for DUs do not support typeDU=| A ofint*mutableint// Not supported, needs to be 'int ref'| B ofint*stringAdditionally, there is a pattern in this codebase where a value is locked via fsharp/src/fsharp/FSharp.Core/seq.fs Lines 1015 to 1021 in e69e4f1 Finally, there are some cases where a mutable value will actually be converted into a ref cell, such as enclosing the value in a lambda function that is also the return value: letf=let mutablex=0fun _ -> x <- x +1letg=letx= ref 0fun _ -> x :=!x +1
f()
f()
g()
g()Both function values are equivalent. This is getting into pretty niche territory though. I'd say that these cases tend to be pretty niche in F# programming, since mutating values is usually done local to a function or method and done for performance reasons. In those cases, |
heronbpv
commented
Jan 3, 2020
Thanks for the answer, @cartermp ! |
KevinRansom
commented
Jan 5, 2020
@cartermp , this is good work. I will mark it approved, and leave it to you to deal with: I think just reverting the change should be good enough. Kevin |
KevinRansom
left a comment
There was a problem hiding this comment.
Subject to you dealing with:
let mutable table = mtyp.ActivePatternElemRefLookupTable
cacheOptRef &table (fun () ->
cartermp
commented
Jan 6, 2020
@KevinRansom Thanks, yeah it's been kinda fun digging through what are surely some of the older parts of the codebase. The |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
I will incorporate parts of #8062 into this |
# This is the 1st commit message: ref -> mutable in more places in the compiler # The commit message dotnet#2 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191229.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19629.1 # The commit message dotnet#3 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191230.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19630.1 # The commit message dotnet#4 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191231.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19631.1 # The commit message dotnet#5 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20200101.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20051.1 # The commit message dotnet#6 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191216.5 (dotnet#8079) # # - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19616.5 # The commit message dotnet#7 will be skipped: # dispose fsi at the end of a scripting session (dotnet#8084) # # The commit message dotnet#8 will be skipped: # Added static link tests and extended CompilerAssert (dotnet#8101) # # * Changed CompilerAssert to static class. Added Compile/Execute methods that take a Compilation description. Added static link tests # # * Hiding compilation description internals # # * Added another test to check for sanity # # * Making a few optional parameters # # * Hiding internals of CompilationReference # The commit message dotnet#9 will be skipped: # Parameterize product version (dotnet#8031) # # * Parameterize Product details # # * fcs # # * Repack pkgdef
forki
commented
Jan 7, 2020
Great work |
cartermp
commented
Jan 7, 2020
Just need to figure out why some parts of absil are so touchy :) |
…otnet#8063) * # This is a combination of 9 commits. # This is the 1st commit message: ref -> mutable in more places in the compiler # The commit message #2 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191229.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19629.1 # The commit message #3 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191230.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19630.1 # The commit message #4 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191231.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19631.1 # The commit message #5 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20200101.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20051.1 # The commit message #6 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191216.5 (dotnet#8079) # # - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19616.5 # The commit message #7 will be skipped: # dispose fsi at the end of a scripting session (dotnet#8084) # # The commit message #8 will be skipped: # Added static link tests and extended CompilerAssert (dotnet#8101) # # * Changed CompilerAssert to static class. Added Compile/Execute methods that take a Compilation description. Added static link tests # # * Hiding compilation description internals # # * Added another test to check for sanity # # * Making a few optional parameters # # * Hiding internals of CompilationReference # The commit message #9 will be skipped: # Parameterize product version (dotnet#8031) # # * Parameterize Product details # # * fcs # # * Repack pkgdef * no ilread
…otnet#8063) * # This is a combination of 9 commits. # This is the 1st commit message: ref -> mutable in more places in the compiler # The commit message #2 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191229.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19629.1 # The commit message #3 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191230.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19630.1 # The commit message #4 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191231.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19631.1 # The commit message #5 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20200101.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20051.1 # The commit message #6 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191216.5 (dotnet#8079) # # - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19616.5 # The commit message #7 will be skipped: # dispose fsi at the end of a scripting session (dotnet#8084) # # The commit message #8 will be skipped: # Added static link tests and extended CompilerAssert (dotnet#8101) # # * Changed CompilerAssert to static class. Added Compile/Execute methods that take a Compilation description. Added static link tests # # * Hiding compilation description internals # # * Added another test to check for sanity # # * Making a few optional parameters # # * Hiding internals of CompilationReference # The commit message #9 will be skipped: # Parameterize product version (dotnet#8031) # # * Parameterize Product details # # * fcs # # * Repack pkgdef * no ilread
…otnet#8063) * # This is a combination of 9 commits. # This is the 1st commit message: ref -> mutable in more places in the compiler # The commit message #2 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191229.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19629.1 # The commit message #3 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191230.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19630.1 # The commit message #4 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191231.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19631.1 # The commit message #5 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20200101.1 # # - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20051.1 # The commit message #6 will be skipped: # Update dependencies from https://github.com/dotnet/arcade build 20191216.5 (dotnet#8079) # # - Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19616.5 # The commit message #7 will be skipped: # dispose fsi at the end of a scripting session (dotnet#8084) # # The commit message #8 will be skipped: # Added static link tests and extended CompilerAssert (dotnet#8101) # # * Changed CompilerAssert to static class. Added Compile/Execute methods that take a Compilation description. Added static link tests # # * Hiding compilation description internals # # * Added another test to check for sanity # # * Making a few optional parameters # # * Hiding internals of CompilationReference # The commit message #9 will be skipped: # Parameterize product version (dotnet#8031) # # * Parameterize Product details # # * fcs # # * Repack pkgdef * no ilread
The large majority of ref cells in the compiler codebase are trivially replaceable with mutable values. A few are not, and so those are mostly left untouched. I think this helps with codebase readability.
Excludes test code