-
Notifications
You must be signed in to change notification settings - Fork 833
Use new Cache api in various places #18872
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
Draft
majocha
wants to merge
42
commits into
dotnet:main
Choose a base branch
from
majocha:cache-2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
6b0f7d5
optimizer
majocha 208362a
argInfo + implicit yield
majocha 4925224
fix comment
majocha 59bb141
Merge branch 'main' into cache-2
majocha bd5a72f
Merge branch 'main' into cache-2
majocha 8d32412
see if this helps
majocha 689112d
wip
majocha 13d4dd1
Revert "wip"
majocha 7543b43
log min threads
majocha 859fdac
?
majocha 873a945
force immediate eviction in some test runs
majocha 03a606d
.
majocha dedb3ee
Merge branch 'main' into cache-2
majocha e6fc866
.
majocha 4d69ae5
format
majocha 942640b
.
majocha d3fd87b
Merge branch 'main' into cache-2
majocha aac2a46
add a test
majocha 57c8f3f
restore optimizer
majocha 078b49f
additional type rel caches
majocha e4d45f4
Merge branch 'main' into cache-2
majocha 128e847
while loop
majocha 3d6c92c
Merge branch 'main' into cache-2
majocha d3dbcd1
Merge branch 'main' into cache-2
majocha 14a7822
Merge branch 'main' into cache-2
majocha f6c3319
don't unload ad
majocha 62dc77f
server gc in netcore tests
majocha 930d871
collect stats better
majocha af601bb
show stats in vs output pane
majocha 66a5ba7
fix method missing at runtime - ns2.0 haha
majocha bd7c24d
format
majocha 8f550ae
Merge branch 'main' into cache-2
majocha c308343
better formatting of stats
majocha db6ff66
Merge branch 'main' into cache-2
majocha 0e32a4b
use immutable array
majocha 8b4738e
MemoizationTable backed by Cache
majocha 2e96d76
fix perf
majocha 314a6e4
format
majocha 3b1124e
Merge branch 'main' into cache-2
majocha 58c0e6b
merge main
majocha 600f619
merge main
majocha e5ba962
fix merge
majocha File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
64 changes: 64 additions & 0 deletions
64
tests/FSharp.Compiler.ComponentTests/Optimizer/NestedApplications.fs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
namespace FSharp.Compiler.ComponentTests.Optimizer | ||
|
||
open System.Text | ||
open Xunit | ||
open FSharp.Test | ||
open FSharp.Test.Compiler | ||
open FSharp.Test.Utilities | ||
|
||
module private Gen = | ||
let nestedLetApps depth = | ||
// Builds: let v1 = id 0 in let v2 = id v1 in ... in ignore vN | ||
let sb = StringBuilder() | ||
sb.AppendLine("module M") |> ignore | ||
sb.AppendLine("let id x = x") |> ignore | ||
sb.AppendLine("let run () =") |> ignore | ||
for i in 1 .. depth do | ||
if i = 1 then | ||
sb.Append(" let v1 = id 0") |> ignore | ||
else | ||
sb.Append(" in let v").Append(i).Append(" = id v").Append(i-1) |> ignore | ||
sb.AppendLine(" in ()") |> ignore | ||
sb.ToString() | ||
|
||
let nestedDirectApps depth = | ||
// Builds: let res = id(id(id(...(0)))) in ignore res | ||
let sb = StringBuilder() | ||
sb.AppendLine("module N") |> ignore | ||
sb.AppendLine("let id x = x") |> ignore | ||
sb.Append("let run () = let res = ") |> ignore | ||
for _ in 1 .. depth do | ||
sb.Append("id (") |> ignore | ||
sb.Append("0") |> ignore | ||
for _ in 1 .. depth do | ||
sb.Append(")") |> ignore | ||
sb.AppendLine(" in ignore res") |> ignore | ||
sb.ToString() | ||
|
||
[<Collection(nameof NotThreadSafeResourceCollection)>] | ||
type ``Nested application optimizer``() = | ||
|
||
// Moderate depths to keep CI stable while still exercising the quadratic shapes | ||
[<Theory>] | ||
[<InlineData(100)>] | ||
[<InlineData(1000)>] | ||
let ``let-chains of nested apps compile under --optimize+`` depth = | ||
let src = Gen.nestedLetApps depth | ||
FSharp src | ||
|> withOptions [ "--optimize+"; "--times" ] | ||
|> asExe | ||
|> ignoreWarnings | ||
|> compile | ||
|> shouldSucceed | ||
|
||
[<Theory>] | ||
[<InlineData(100)>] | ||
[<InlineData(1000)>] | ||
let ``direct nested application compiles under --optimize+`` depth = | ||
let src = Gen.nestedDirectApps depth | ||
FSharp src | ||
|> withOptions [ "--optimize+"; "--times" ] | ||
|> asExe | ||
|> ignoreWarnings | ||
|> compile | ||
|> shouldSucceed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semantics w.r.t. to error handling is different now, aren't they?
Also it feels like the previous version had a race condition (
FindAll
and laterAdd
), possibly theMulti
map was chosen to workaround it by storing a list and not a single value?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous version had some concurrency problems, because Vlad at some point added
ConcurrentDictionary
asHashMultiMap
backing store to fix them.Semantics are different. Originally this just always removed the value in
finally
. I have to revisit this, I remember the change made sense to me but I forgot why, oh lol. I guess my thinking was eviction will be sufficient here. Now I also notice this was attached toenv
, notcenv
.This is not really tested in the test suite but there is a benchmark. I'll run it later to see if this still works.