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

use ternery instead of ifelse since ifelse force evaluation of the RHS #78

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

KristofferC
Copy link
Sponsor Member

Made on top of #77 to avoid some merge conflicts and have up to date benchmarks.

I was looking at why the following benchmark allocates:

julia> using StyledStrings

julia> styles = Pair{Symbol, Any}[:face => :green]
1-element Vector{Pair{Symbol, Any}}:
 :face => :green

julia> face = StyledStrings.getface(styles);

julia> using BenchmarkTools

julia> @btime merge($face, $face)
  19.266 ns (2 allocations: 32 bytes)

With the following PR we instead get

julia> @btime merge($face, $face)
  13.388 ns (0 allocations: 0 bytes)

The benchmark in #75

goes from

  8.654 ms (202611 allocations: 16.62 MiB)

to

  7.972 ms (148360 allocations: 15.72 MiB)

Not a huge speedup but quite a big reduction in allocations which might be useful as other things improve.

I am not 100% sure why this is needed but I typically find the compiler optimizes better with ternary ? over ifelse.

@tecosaur
Copy link
Collaborator

tecosaur commented Aug 6, 2024

Interesting, since there's no computation to be done I simply assumed that ifelse would be equivalent to an actual branch.

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Aug 6, 2024

This is just a guess but the field on the RHS for e.g. underline is not very well-typed so accessing that could have some cost? Not sure..

While ifelse looked like a reasonable choice, it turns out that the
performance (allocations in particular) is improved by creating a
branch. We go from 2 allocations to 0 when merging faces, which very
much adds up over time.
@tecosaur tecosaur merged commit 9b9cf71 into main Aug 6, 2024
5 checks passed
ifelse(isnothing(b.underline), a.underline, b.underline),
ifelse(isnothing(b.strikethrough), a.strikethrough, b.strikethrough),
ifelse(isnothing(b.inverse), a.inverse, b.inverse),
abheight = if isnothing(bheight) aheight
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

FWIW, I would write this something like

abheight = isnothing(bheight) ? aheight :
           isnothing(aheight) ? bheight :
           bheight isa Int    ? bheight :
           aheight isa Int    ? round(Int, aheight * bheight) :
                                aheight * bheight

I haven't really seen Julia code that puts the body of an if on the same line as the condition, instead people typically use terneries in cases like this from my experience.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have seen that pattern a bit, but I can't say I'm a fan of it. Perhaps it's a bit selfish of me, but I'm rather a fan of (if cond val otherwise) and (cond ...) forms and so like to use them over ternaries in projects where I'm the primary author.

I'm also a fan of consistency, and there are nine other one-line if ... else ... end statements floating around already.

@tecosaur tecosaur deleted the kc/ternery_ifelse branch August 6, 2024 17:34
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