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

Cache default font #210

Merged
merged 10 commits into from
Feb 21, 2022
Merged

Cache default font #210

merged 10 commits into from
Feb 21, 2022

Conversation

ederag
Copy link
Contributor

@ederag ederag commented Feb 8, 2022

Caching default font is the last bit needed to accelerate savegif from 12s to 3.4s (fully fixes #194).
The timings now are

sim GifOutput:   3.543932 seconds (15.66 k allocations: 369.376 MiB, 2.02% gc time)
sim ArrayOutput:   0.486234 seconds (459 allocations: 2.530 MiB)
savegif:   3.139735 seconds (16.19 k allocations: 492.624 MiB, 0.02% gc time)

set_default_font(FTFont(path)) can be used to bypass any findfont call, if desired.
Please let me know if this is OK to export (I'd add a docstring then).

src/outputs/textconfig.jl Outdated Show resolved Hide resolved
@rafaqz
Copy link
Member

rafaqz commented Feb 8, 2022

Thanks! We probably don't need to export, as DynamicGrids.set_default_font has much clearer meaning than set_default_font, and I guess it will be rarely used and then only once.

But you can add a docstring so its part of the unexported interface. Just include it using the full DynamicGrids.set_default_font string where it seems to fit in the docs.

@ederag
Copy link
Contributor Author

ederag commented Feb 11, 2022

Fully agreed, thanks !
It should be ready for further review.

@ederag
Copy link
Contributor Author

ederag commented Feb 12, 2022

On second thoughts, font-related error handlers needed an update
(now that FTFont and set_default_font can be used)
=>

julia> DynamicGrids._fontwrongtype(1)
ERROR: ArgumentError: font must be either a String or a FreeTypeAbstraction.FTFont,
got `1` which is a `Int64`.
...

julia> DynamicGrids._fontnotfounderror("cantarell")
ERROR: ArgumentError: Font "cantarell" wasn't found in this system.
Specify an existing font with the `font` keyword,
or call DynamicGrids.set_default_font first,
or use `text=nothing` to display no text.
...

julia> DynamicGrids._nodefaultfonterror(("cantarell", "sans-serif", "Bookman"))
ERROR: Your system does not contain any of the default fonts
("cantarell", "sans-serif", "Bookman").
Use DynamicGrids.set_default_font first.
...

The last commit is a proposition to move those error handlers to the end
(the beginning logic is then easier to grasp),
but I have no strong opinion about it.

Copy link
Member

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry I took so long to review this!

@noinline _fontnotfounderror(font) =
throw(ArgumentError(
"""
Font "$font" wasn't found in this system.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason these are wrapped so narrow now? 92 chars is the standard max width for Julia.

Copy link
Contributor Author

@ederag ederag Feb 21, 2022

Choose a reason for hiding this comment

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

This was an application of "one chunk of thought per line",
which makes things much easier to read.
I agree that the newline before throw was unnecessary, a9dbfc8 is better ?

@rafaqz rafaqz merged commit 535b821 into cesaraustralia:master Feb 21, 2022
@ederag ederag deleted the savegif branch February 22, 2022 07:47
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.

savegif slower than GifOutput
2 participants