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

remove string("a", 'b') regression. This shouldn't work #50891

Closed

Conversation

oscardssmith
Copy link
Member

Fixes #50458, #49249 but I have no idea why.
Before:

@btime string("a", 'b')
  246.945 ns (6 allocations: 184 bytes)

After:

@btime string("a", 'b')
18.195 ns (1 allocation: 24 bytes)

Can anyone explain this? It feels like a compiler bug to me.

@oscardssmith oscardssmith added performance Must go faster backport 1.10 Change should be backported to the 1.10 release labels Aug 11, 2023
@Seelengrab
Copy link
Contributor

Just a guess, but this reads like removing the type annotation leads to better specialization due to the Vararg possibly getting a narrower type?

@nsajko
Copy link
Contributor

nsajko commented Aug 12, 2023

Pretty sure this is expected behavior, see: https://docs.julialang.org/en/v1/manual/performance-tips/#Be-aware-of-when-Julia-avoids-specializing

As a heuristic, Julia avoids automatically specializing on argument type parameters in three specific cases: Type, Function, and Vararg.

@Seelengrab
Copy link
Contributor

But then the change here ought to not make a difference 🤔

@nsajko
Copy link
Contributor

nsajko commented Aug 12, 2023

It's weird though, if I define a function like h(z::Vararg{Union{Int8, Int16, Int32, Int64}}) = +(z...) (a four member union, just like before this PR), the specialization happens fine judging by @code_warntype h(one(Int8), one(Int16)).

@Seelengrab
Copy link
Contributor

@code_warntype is lying to you, as is mentioned just a few lines below the line you linked:

Note that @code_typed and friends will always show you specialized code, even if Julia would not normally specialize that method call. You need to check the method internals if you want to see whether specializations are generated when argument types are changed, i.e., if (@which f(...)).specializations contains specializations for the argument in question.

@KristofferC
Copy link
Member

Should probably tweak the commit message. There should be a chance to understand the change by reading just the commit message.

@JeffBezanson
Copy link
Member

@vtjnash Shouldn't we root-cause this? Seems like a bad bug somewhere. This function doesn't even have multiple methods.

@gbaraldi
Copy link
Member

gbaraldi commented Aug 14, 2023

I'll try and see if it's at least bisectable, not the regression here but the weird behaviour we are seeing.

@oscardssmith
Copy link
Member Author

#50929 replaces this (assuming it's backportable)

@IanButterworth IanButterworth removed the backport 1.10 Change should be backported to the 1.10 release label Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance regression in "spellcheck" string processing benchmark
8 participants