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.

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

reduce allocations #1207

Closed
wants to merge 9 commits into from
Closed

reduce allocations #1207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a23a52c
reduce allocations
vasily-kirichenko May 19, 2016
32673e4
Remove tuples in primValRefEq
forki May 21, 2016
5757164
Merge pull request #1 from forki/optimizations
vasily-kirichenko May 21, 2016
c6be69d
Fix bug in error reporting
forki May 21, 2016
2f9a55b
Merge pull request #2 from forki/tuplealloc
vasily-kirichenko May 21, 2016
453c95d
refactor CheckDispatchSlotsAreImplemented
vasily-kirichenko May 21, 2016
955a726
rollback refactor CheckDispatchSlotsAreImplemented
vasily-kirichenko May 22, 2016
417e75d
fix condition
vasily-kirichenko May 22, 2016
828dfab
change fsc.exe GCLatencyMode to LowLatency as it showed better perfor…
vasily-kirichenko May 22, 2016
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
10 changes: 4 additions & 6 deletions src/fsharp/MethodOverrides.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ module DispatchSlotChecking =
match overrides |> List.filter (IsPartialMatch g amap m dispatchSlot) with
| [] ->
match overrides |> List.filter (fun overrideBy -> IsNameMatch dispatchSlot overrideBy &&
IsImplMatch g dispatchSlot overrideBy) with
IsImplMatch g dispatchSlot overrideBy) with
| [] ->
noimpl()
| [ Override(_,_,_,(mtps,_),argTys,_,_,_) as overrideBy ] ->
Expand All @@ -307,13 +307,11 @@ module DispatchSlotChecking =
errorR(Error(FSComp.SR.typrelOverloadNotFound(FormatMethInfoSig g amap m denv dispatchSlot, FormatMethInfoSig g amap m denv dispatchSlot),overrideBy.Range))

| [ overrideBy ] ->

match dispatchSlots |> List.filter (fun (RequiredSlot(dispatchSlot,_)) -> OverrideImplementsDispatchSlot g amap m dispatchSlot overrideBy) with
| [] ->
if dispatchSlots |> List.exists (fun (RequiredSlot(dispatchSlot,_)) -> OverrideImplementsDispatchSlot g amap m dispatchSlot overrideBy) then
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong! there should be a |> not

Copy link
Contributor

Choose a reason for hiding this comment

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

can we bind to a variable, the if condition is super long?

noimpl()
else
// Error will be reported below in CheckOverridesAreAllUsedOnce
()
| _ ->
noimpl()

| _ ->
fail(Error(FSComp.SR.typrelOverrideWasAmbiguous(FormatMethInfoSig g amap m denv dispatchSlot),m))
Expand Down
2 changes: 1 addition & 1 deletion src/fsharp/fscmain.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ do ()

[<EntryPoint>]
let main(argv) =
System.Runtime.GCSettings.LatencyMode <- System.Runtime.GCLatencyMode.Batch
System.Runtime.GCSettings.LatencyMode <- System.Runtime.GCLatencyMode.LowLatency
use unwindBuildPhase = PushThreadBuildPhaseUntilUnwind BuildPhase.Parameter

#if NO_HEAPTERMINATION
Expand Down
25 changes: 11 additions & 14 deletions src/fsharp/tast.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4398,19 +4398,18 @@ let fslibValRefEq fslibCcu vref1 vref2 =
/// This takes into account the possibility that they may have type forwarders
let primEntityRefEq compilingFslib fslibCcu (x : EntityRef) (y : EntityRef) =
x === y ||
match x.IsResolved,y.IsResolved with
| true, true when not compilingFslib -> x.ResolvedTarget === y.ResolvedTarget
| _ ->
match x.IsLocalRef,y.IsLocalRef with
| false, false when

if x.IsResolved && y.IsResolved && not compilingFslib then
Copy link
Contributor

Choose a reason for hiding this comment

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

the same stuff is happening couple of lines down in primValRefEq - and that method shows up in hot path when I compile Paket.Core

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, then yes, that should also be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

we have this fix included as well.

x.ResolvedTarget === y.ResolvedTarget
elif not x.IsLocalRef && not y.IsLocalRef &&
(// Two tcrefs with identical paths are always equal
nonLocalRefEq x.nlr y.nlr ||
// The tcrefs may have forwarders. If they may possibly be equal then resolve them to get their canonical references
// and compare those using pointer equality.
(not (nonLocalRefDefinitelyNotEq x.nlr y.nlr) && x.Deref === y.Deref)) ->
(not (nonLocalRefDefinitelyNotEq x.nlr y.nlr) && x.Deref === y.Deref)) then
true
| _ ->
compilingFslib && fslibEntityRefEq fslibCcu x y
else
Copy link
Contributor

Choose a reason for hiding this comment

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

style only: elif to avoid nesting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

compilingFslib && fslibEntityRefEq fslibCcu x y

/// Primitive routine to compare two UnionCaseRef's for equality
let primUnionCaseRefEq compilingFslib fslibCcu (UCRef(tcr1,c1) as uc1) (UCRef(tcr2,c2) as uc2) =
Expand All @@ -4425,12 +4424,10 @@ let primUnionCaseRefEq compilingFslib fslibCcu (UCRef(tcr1,c1) as uc1) (UCRef(tc
/// Note this routine doesn't take type forwarding into account
let primValRefEq compilingFslib fslibCcu (x : ValRef) (y : ValRef) =
x === y ||
match x.IsResolved,y.IsResolved with
| true, true when x.ResolvedTarget === y.ResolvedTarget -> true
| _ ->
match x.IsLocalRef,y.IsLocalRef with
| true,true when valEq x.PrivateTarget y.PrivateTarget -> true
| _ ->
if (x.IsResolved && y.IsResolved && x.ResolvedTarget === y.ResolvedTarget) ||
(x.IsLocalRef && y.IsLocalRef && valEq x.PrivateTarget y.PrivateTarget) then
true
else
(// Use TryDeref to guard against the platforms/times when certain F# language features aren't available,
// e.g. CompactFramework doesn't have support for quotations.
let v1 = x.TryDeref
Expand Down
14 changes: 7 additions & 7 deletions src/utils/prim-lexing.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,9 @@ namespace Internal.Utilities.Text.Lexing
if lexBuffer.IsPastEndOfStream then failwith "End of file on lexing stream";
lexBuffer.IsPastEndOfStream <- true;
//printf "state %d --> %d on eof\n" state snew;
scanUntilSentinel(lexBuffer,snew)
scanUntilSentinel lexBuffer snew
else
scanUntilSentinel(lexBuffer, state)
scanUntilSentinel lexBuffer state

let onAccept (lexBuffer:LexBuffer<char>,a) =
lexBuffer.LexemeLength <- lexBuffer.BufferScanLength;
Expand All @@ -201,7 +201,7 @@ namespace Internal.Utilities.Text.Lexing
let numUnicodeCategories = 30
let numLowUnicodeChars = 128
let numSpecificUnicodeChars = (trans.[0].Length - 1 - numLowUnicodeChars - numUnicodeCategories)/2
let lookupUnicodeCharacters (state,inp) =
let lookupUnicodeCharacters state inp =
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is harmless but AFAICS doesn't alter the representation or calls of the function? e.g. for

type C() = 
   let f (x,y) = x + y
   member a.M(b,c) = f (b,c)

we get

.method assembly hidebysig instance int32 
        f(int32 x,
          int32 y) cil managed

Copy link
Contributor

Choose a reason for hiding this comment

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

@vasily-kirichenko Could you remove the changes in prim-lexing.fs please? I'm pretty sure they don't remove any allocations. (If they do then let's discuss further, there must be something I'm missing). Thanks!!

let inpAsInt = int inp
// Is it a fast ASCII character?
if inpAsInt < numLowUnicodeChars then
Expand Down Expand Up @@ -235,7 +235,7 @@ namespace Internal.Utilities.Text.Lexing
loop 0
let eofPos = numLowUnicodeChars + 2*numSpecificUnicodeChars + numUnicodeCategories

let rec scanUntilSentinel(lexBuffer,state) =
let rec scanUntilSentinel lexBuffer state =
// Return an endOfScan after consuming the input
let a = int accept.[state]
if a <> sentinel then
Expand All @@ -251,14 +251,14 @@ namespace Internal.Utilities.Text.Lexing
let inp = lexBuffer.Buffer.[lexBuffer.BufferScanPos]

// Find the new state
let snew = lookupUnicodeCharacters (state,inp)
let snew = lookupUnicodeCharacters state inp

if snew = sentinel then
lexBuffer.EndOfScan()
else
lexBuffer.BufferScanLength <- lexBuffer.BufferScanLength + 1;
//printf "state %d --> %d on '%c' (%d)\n" s snew (char inp) inp;
scanUntilSentinel(lexBuffer,snew)
scanUntilSentinel lexBuffer snew

// Each row for the Unicode table has format
// 128 entries for ASCII characters
Expand All @@ -268,6 +268,6 @@ namespace Internal.Utilities.Text.Lexing

member tables.Interpret(initialState,lexBuffer : LexBuffer<char>) =
startInterpret(lexBuffer)
scanUntilSentinel(lexBuffer, initialState)
scanUntilSentinel lexBuffer initialState

static member Create(trans,accept) = new UnicodeTables(trans,accept)