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

Some small optimizations (helping type inference, avoid eager int to string conversion) #75

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

KristofferC
Copy link
Member

Benchmark used:

using Random
using StyledStrings

function generate_annot_string(n)
    colors = [:yellow, :green, :blue, :red]
    offset = 1
    annotations = Tuple{UnitRange{Int64}, Pair{Symbol, Any}}[]
    while true
        col = rand(1:length(colors))
        l = rand(1:10)
        offset + l > n && break
        push!(annotations, (offset:(offset+l), :face => colors[col]))
        offset += l
    end
    s = randstring(n)
    return Base.AnnotatedString(s, annotations)
end

str = generate_annot_string(100000)

using BenchmarkTools

@btime show(IOContext(devnull, :color => true), MIME("text/plain"), str)

Baseline:
27.456 ms (554582 allocations: 26.87 MiB)

First commit
13.629 ms (412776 allocations: 20.65 MiB)

Second commit:
12.911 ms (408186 allocations: 20.46 MiB)

Third commit:
9.508 ms (290571 allocations: 18.78 MiB)

@KristofferC KristofferC force-pushed the kc/inference_merge branch 2 times, most recently from c3ea592 to e28f6ad Compare August 6, 2024 10:44
When `background` is set the background variant will be provided, otherwise
the provided code is for setting the foreground color.
"""
function ansi_4bit_color_code(color::Symbol, background::Bool=false)
if haskey(ANSI_4BIT_COLORS, color)
code = ANSI_4BIT_COLORS[color]
code = get(ANSI_4BIT_COLORS, color, nothing)
Copy link
Member Author

Choose a reason for hiding this comment

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

This pattern avoids doing the dict lookup twice.

@@ -519,6 +523,11 @@ Base.merge(a::Face, b::Face, others::Face...) = merge(merge(a, b), others...)

## Getting the combined face from a set of properties ##

# Putting these inside `getface` causes the julia compiler to box it
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't actually know why this happens, it's just that doing this change fixes the issue reported by @code_warntype... I'm kind of scared of closures for reasons like this :(

@tecosaur
Copy link
Collaborator

tecosaur commented Aug 6, 2024

This all looks good to go from my end. If there's nothing else that you're thinking of adding to this, I'm tempted to just rebase + tweak a few of the commit messages then merge 🙂

@KristofferC
Copy link
Member Author

Sounds good to me!

Help Julia's type inference by extracting out a field access to outside
type checks.

Running a benchmark on a large, heavily annotated string we see a 50%
reduction in runtime (27.5ms -> 13.6ms) and a 25% reduction in
allocations (554k -> 412k).
These will just be printed anyway, converting a number to a string to
immediately print it is unnecessary.

Running a benchmark on a large, heavily annotated string we see a 5%
reduction in runtime (13.6ms -> 12.9) and a 1% reduction in
allocations (412k -> 408k).
Running a benchmark on a large, heavily annotated string we see a 25%
reduction in runtime (12.9ms -> 9.5ms) and a 29% reduction in
allocations (408k -> 290k).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants