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.

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

Struct-ify a bunch of internals #16316

Closed
wants to merge 17 commits into from

Conversation

vzarytovskii
Copy link
Member

@vzarytovskii vzarytovskii commented Nov 21, 2023

I see a decrease of working set in running standalone compiler ~3650 -> ~3400, should be even more beneficial for long-running stuff.

@vzarytovskii vzarytovskii changed the title ValueOption-ify a bunch of internals Struct-ify a bunch of internals Nov 29, 2023
Comment on lines 516 to +565
// Strip a bunch of leading '>' of a token, at the end of a typar application
// Note: this is used in the 'service.fs' to do limited postprocessing
[<return: Struct>]
let (|TyparsCloseOp|_|) (txt: string) =
let angles = txt |> Seq.takeWhile (fun c -> c = '>') |> Seq.toList
let afterAngles = txt |> Seq.skipWhile (fun c -> c = '>') |> Seq.toList
if List.isEmpty angles then None else
if List.isEmpty angles then ValueNone else

let afterOp =
match (System.String(Array.ofSeq afterAngles)) with
| "." -> Some DOT
| "]" -> Some RBRACK
| "-" -> Some MINUS
| ".." -> Some DOT_DOT
| "?" -> Some QMARK
| "??" -> Some QMARK_QMARK
| ":=" -> Some COLON_EQUALS
| "::" -> Some COLON_COLON
| "*" -> Some STAR
| "&" -> Some AMP
| "->" -> Some RARROW
| "<-" -> Some LARROW
| "=" -> Some EQUALS
| "<" -> Some (LESS false)
| "$" -> Some DOLLAR
| "%" -> Some (PERCENT_OP("%") )
| "%%" -> Some (PERCENT_OP("%%"))
| "" -> None
| "." -> ValueSome DOT
| "]" -> ValueSome RBRACK
| "-" -> ValueSome MINUS
| ".." -> ValueSome DOT_DOT
| "?" -> ValueSome QMARK
| "??" -> ValueSome QMARK_QMARK
| ":=" -> ValueSome COLON_EQUALS
| "::" -> ValueSome COLON_COLON
| "*" -> ValueSome STAR
| "&" -> ValueSome AMP
| "->" -> ValueSome RARROW
| "<-" -> ValueSome LARROW
| "=" -> ValueSome EQUALS
| "<" -> ValueSome (LESS false)
| "$" -> ValueSome DOLLAR
| "%" -> ValueSome (PERCENT_OP("%") )
| "%%" -> ValueSome (PERCENT_OP("%%"))
| "" -> ValueNone
| s ->
match List.ofSeq afterAngles with
| '=' :: _
| '!' :: '=' :: _
| '<' :: _
| '>' :: _
| '$' :: _ -> Some (INFIX_COMPARE_OP s)
| '&' :: _ -> Some (INFIX_AMP_OP s)
| '|' :: _ -> Some (INFIX_BAR_OP s)
| '$' :: _ -> ValueSome (INFIX_COMPARE_OP s)
| '&' :: _ -> ValueSome (INFIX_AMP_OP s)
| '|' :: _ -> ValueSome (INFIX_BAR_OP s)
| '!' :: _
| '?' :: _
| '~' :: _ -> Some (PREFIX_OP s)
| '~' :: _ -> ValueSome (PREFIX_OP s)
| '@' :: _
| '^' :: _ -> Some (INFIX_AT_HAT_OP s)
| '^' :: _ -> ValueSome (INFIX_AT_HAT_OP s)
| '+' :: _
| '-' :: _ -> Some (PLUS_MINUS_OP s)
| '*' :: '*' :: _ -> Some (INFIX_STAR_STAR_OP s)
| '-' :: _ -> ValueSome (PLUS_MINUS_OP s)
| '*' :: '*' :: _ -> ValueSome (INFIX_STAR_STAR_OP s)
| '*' :: _
| '/' :: _
| '%' :: _ -> Some (INFIX_STAR_DIV_MOD_OP s)
| _ -> None
Some([| for _c in angles do yield GREATER |], afterOp)
| '%' :: _ -> ValueSome (INFIX_STAR_DIV_MOD_OP s)
| _ -> ValueNone
ValueSome([| for _c in angles do yield GREATER |], afterOp)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you're mostly just swapping option for voption in this PR, and this probably isn't called in a hot path anyway, but in the spirit of reducing allocations, I think this bit could also become something like this:1

open System

[<return: Struct>]
let (|Equals|_|) (s: string) (span: ReadOnlySpan<char>) =
    if span.SequenceEqual(s.AsSpan()) then ValueSome Equals
    else ValueNone

[<return: Struct>]
let (|StartsWith|_|) (s: string) (span: ReadOnlySpan<char>) =
    if span.StartsWith(s.AsSpan()) then ValueSome StartsWith
    else ValueNone

[<return: Struct>]
let (|TyparsCloseOp|_|) (txt: string) =
    let angles = txt.AsSpan().IndexOfAnyExcept '>'
    if angles < 0 then ValueNone else
    let afterAngles = txt.AsSpan angles

    let afterOp =
        match afterAngles with
        | Equals "." -> ValueSome DOT
        | Equals "]" -> ValueSome RBRACK
        | Equals "-" -> ValueSome MINUS
        | Equals ".." -> ValueSome DOT_DOT
        | Equals "?" -> ValueSome QMARK
        | Equals "??" -> ValueSome QMARK_QMARK
        | Equals ":=" -> ValueSome COLON_EQUALS
        | Equals "::" -> ValueSome COLON_COLON
        | Equals "*" -> ValueSome STAR
        | Equals "&" -> ValueSome AMP
        | Equals "->" -> ValueSome RARROW
        | Equals "<-" -> ValueSome LARROW
        | Equals "=" -> ValueSome EQUALS
        | Equals "<" -> ValueSome (LESS false)
        | Equals "$" -> ValueSome DOLLAR
        | Equals "%" -> ValueSome (PERCENT_OP "%")
        | Equals "%%" -> ValueSome (PERCENT_OP "%%")
        | StartsWith "="
        | StartsWith "!="
        | StartsWith "<"
        | StartsWith ">"
        | StartsWith "$" -> ValueSome (INFIX_COMPARE_OP (afterAngles.ToString()))
        | StartsWith "&" -> ValueSome (INFIX_AMP_OP (afterAngles.ToString()))
        | StartsWith "|" -> ValueSome (INFIX_BAR_OP (afterAngles.ToString()))
        | StartsWith "!"
        | StartsWith "?"
        | StartsWith "~"  -> ValueSome (PREFIX_OP (afterAngles.ToString()))
        | StartsWith "@"
        | StartsWith "^" -> ValueSome (INFIX_AT_HAT_OP (afterAngles.ToString()))
        | StartsWith "+"
        | StartsWith "-" -> ValueSome (PLUS_MINUS_OP (afterAngles.ToString()))
        | StartsWith "**" -> ValueSome (INFIX_STAR_STAR_OP (afterAngles.ToString()))
        | StartsWith "*"
        | StartsWith "/"
        | StartsWith "%" -> ValueSome (INFIX_STAR_DIV_MOD_OP (afterAngles.ToString()))
        | _ -> ValueNone

    ValueSome(Array.init angles (fun _ -> GREATER), afterOp)

(It sure would be great to have pattern-matching on spans and a spread operator, eh...)

The above does give a significant boost in microbenchmarks:

| Method      | Input                | Mean      | Error     | StdDev    | Ratio | RatioSD | Gen0   | Allocated | Alloc Ratio |
|------------ |--------------------- |----------:|----------:|----------:|------:|--------:|-------:|----------:|------------:|
| Option      | >=                   | 147.57 ns |  2.966 ns |  3.297 ns |  1.00 |    0.00 | 0.0567 |     712 B |        1.00 |
| ValueOption | >=                   | 138.22 ns |  2.776 ns |  3.609 ns |  0.94 |    0.03 | 0.0534 |     672 B |        0.94 |
| Simplified  | >=                   |  20.06 ns |  0.189 ns |  0.158 ns |  0.14 |    0.00 | 0.0057 |      72 B |        0.10 |
|             |                      |           |           |           |       |         |        |           |             |
| Option      | >>>!=(...)..... [23] | 458.55 ns |  3.048 ns |  2.702 ns |  1.00 |    0.00 | 0.1268 |    1592 B |        1.00 |
| ValueOption | >>>!=(...)..... [23] | 451.80 ns |  2.160 ns |  1.804 ns |  0.99 |    0.01 | 0.1235 |    1552 B |        0.97 |
| Simplified  | >>>!=(...)..... [23] |  40.52 ns |  0.482 ns |  0.402 ns |  0.09 |    0.00 | 0.0147 |     184 B |        0.12 |
|             |                      |           |           |           |       |         |        |           |             |
| Option      | >>>>>>>>>>>>>>:=     | 515.45 ns |  4.525 ns |  3.778 ns |  1.00 |    0.00 | 0.1268 |    1600 B |        1.00 |
| ValueOption | >>>>>>>>>>>>>>:=     | 521.80 ns | 10.311 ns | 11.874 ns |  1.02 |    0.03 | 0.1240 |    1560 B |        0.97 |
| Simplified  | >>>>>>>>>>>>>>:=     |  35.21 ns |  0.302 ns |  0.268 ns |  0.07 |    0.00 | 0.0140 |     176 B |        0.11 |

Footnotes

  1. You'd need to add a polyfill like this for the single-char overload of `IndexOfAnyExcept` to `Utilities/ReadOnlySpan.fs`:
    [<Extension>]
    static member IndexOfAnyExcept(span: ReadOnlySpan<char>, value: char) =
        let mutable i = 0
        let mutable found = false
    
        while not found && i < span.Length do
            let c = span[i]
    
            if c <> value then found <- true else i <- i + 1
    
        if found then i else -1
    

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Dec 8, 2023

There are clearly some problems with stack space with these changes in recursion-heavy places in compiler (we were hitting SOE on desktop CLR in some of the async cases when type checking), will need more thorough investigation. I will close the PR (was meant to be as an experiment to see CI anyway), but will leave the branch, when we'll come back to it. Most of the work here is anyway just mechanical changes to types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants