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

Remove massive string allocations #5940

Merged
merged 5 commits into from
Nov 20, 2018
Merged

Remove massive string allocations #5940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ba7a75
remove massive string allocations
AviAvni Nov 20, 2018
d1660ff
change NormalizedSource Source and Seq to Array computation expression
AviAvni Nov 20, 2018
7b10b82
fix line end according to the source positions
AviAvni Nov 20, 2018
507cbe0
remove pos use i
AviAvni Nov 20, 2018
68983b7
fix some tests
AviAvni Nov 20, 2018
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
18 changes: 9 additions & 9 deletions src/fsharp/CheckFormatStrings.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,15 @@ let parseFormatStringInternal (m:range) (g: TcGlobals) (context: FormatStringChe
let (offset, fmt) =
match context with
| Some context ->
let length = context.NormalizedSource.Length
if m.EndLine < context.LineEndPositions.Length then
let startIndex = context.LineEndPositions.[m.StartLine-1] + m.StartColumn
let endIndex = context.LineEndPositions.[m.EndLine-1] + m.EndColumn - 1
if startIndex < length-3 && context.NormalizedSource.[startIndex..startIndex+2] = "\"\"\"" then
(3, context.NormalizedSource.[startIndex+3..endIndex-3])
elif startIndex < length-2 && context.NormalizedSource.[startIndex..startIndex+1] = "@\"" then
(2, context.NormalizedSource.[startIndex+2..endIndex-1])
else (1, context.NormalizedSource.[startIndex+1..endIndex-1])
let length = context.Source.Length
if m.EndLine < context.LineStartPositions.Length then
let startIndex = context.LineStartPositions.[m.StartLine-1] + m.StartColumn
let endIndex = context.LineStartPositions.[m.EndLine-1] + m.EndColumn - 1
if startIndex < length-3 && context.Source.[startIndex..startIndex+2] = "\"\"\"" then
cartermp marked this conversation as resolved.
Show resolved Hide resolved
(3, context.Source.[startIndex+3..endIndex-3])
elif startIndex < length-2 && context.Source.[startIndex..startIndex+1] = "@\"" then
(2, context.Source.[startIndex+2..endIndex-1])
else (1, context.Source.[startIndex+1..endIndex-1])
else (1, fmt)
| None -> (1, fmt)

Expand Down
22 changes: 13 additions & 9 deletions src/fsharp/NameResolution.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1251,8 +1251,8 @@ type OpenDeclaration =
IsOwnNamespace = isOwnNamespace }

type FormatStringCheckContext =
{ NormalizedSource: string
LineEndPositions: int[] }
{ Source: string
LineStartPositions: int[] }

/// An abstract type for reporting the results of name resolution and type checking.
type ITypecheckResultsSink =
Expand Down Expand Up @@ -1538,14 +1538,18 @@ type TcResultsSinkImpl(g, ?source: string) =
let formatStringCheckContext =
lazy
source |> Option.map (fun source ->
let source = source.Replace("\r\n", "\n").Replace("\r", "\n")
let positions =
source.Split('\n')
|> Seq.map (fun s -> String.length s + 1)
|> Seq.scan (+) 0
|> Seq.toArray
{ NormalizedSource = source
LineEndPositions = positions })
[|
Copy link

Choose a reason for hiding this comment

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

Using Seq.pairwise might be more elegant here, rahter than indexing in arrays. (Though I'm not sure if perf would be better or worse taking into account possible compiler optimization and CPU caching).

yield 0
Copy link

Choose a reason for hiding this comment

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

Possible bug: So suppose the file content is just the 1 byte character \n. This will give for positions:
[| 0; 1; 1 |] Should not we want just [| 0; 1 |] instead? CC: @forki @AviAvni

for i in 0..source.Length-1 do
Copy link
Contributor

Choose a reason for hiding this comment

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

i and pos are always in sync right? So you can get rid of that mutable variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry thanks

let c = source.[i]
if c = '\r' && i + 1 < source.Length && source.[i+1] = '\n' then ()
Copy link

Choose a reason for hiding this comment

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

style/readbility comment: looping through 1..source.Length instead of 0..source.Length-1 would make more sense here, since we already processed the 0th byte with the yield 0 above the loop. This would also avoid the i+1 in four places:

 [|
    yield 0
    for i in 1..source.Length do
         let c = source.[i-1]
         if c = '\r' && i < source.Length && source.[i] = '\n' then ()
         elif c = '\r' then yield i
         elif c = '\n' then yield i
    yield source.Length
 |]

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think we should avoid duplicates as well.

elif c = '\r' then yield i + 1
if c = '\n' then yield i + 1
Copy link

Choose a reason for hiding this comment

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

Should not this be an elif? (It does not change the semantics but improves clarity.) @AviAvni

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 it's fine. alter native would be to pattern match on c

yield source.Length
|]
{ Source = source
LineStartPositions = positions })

member this.GetResolutions() =
TcResolutions(capturedEnvs, capturedExprTypings, capturedNameResolutions, capturedMethodGroupResolutions)
Expand Down
10 changes: 5 additions & 5 deletions src/fsharp/NameResolution.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,12 +344,12 @@ type internal OpenDeclaration =
/// Create a new instance of OpenDeclaration.
static member Create : longId: Ident list * modules: ModuleOrNamespaceRef list * appliedScope: range * isOwnNamespace: bool -> OpenDeclaration

/// Line-end normalized source text and an array of line end positions, used for format string parsing
/// Source text and an array of line end positions, used for format string parsing
type FormatStringCheckContext =
{ /// Line-end normalized source text
NormalizedSource: string
/// Array of line end positions
LineEndPositions: int[] }
{ /// Source text
Source: string
/// Array of line start positions
LineStartPositions: int[] }

/// An abstract type for reporting the results of name resolution and type checking
type ITypecheckResultsSink =
Expand Down
3 changes: 1 addition & 2 deletions tests/service/EditorTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,7 @@ let _ = printf " %*a" 3 (fun _ _ -> ()) 2
typeCheckResults.GetFormatSpecifierLocationsAndArity()
|> Array.map (fun (range,numArgs) -> range.StartLine, range.StartColumn, range.EndLine, range.EndColumn, numArgs)
|> shouldEqual
[|(2, 45, 2, 47, 1); (3, 23, 3, 25, 1); (4, 38, 4, 40, 1); (5, 27, 5, 29
, 1);
[|(2, 45, 2, 47, 1); (3, 23, 3, 25, 1); (4, 38, 4, 40, 1); (5, 27, 5, 29, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just remove line break

(6, 17, 6, 20, 2); (7, 17, 7, 22, 1); (8, 17, 8, 23, 1); (9, 18, 9, 22, 1);
(10, 18, 10, 21, 1); (12, 12, 12, 15, 1); (15, 12, 15, 15, 1);
(16, 28, 16, 30, 1); (18, 30, 18, 32, 1); (19, 30, 19, 32, 1);
Expand Down