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.

Sign up for GitHub

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

Jump to bottom

broadcast: disable nospecialize logic for outer method signature #43200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

broadcast: disable nospecialize logic for outer method signature #43200

wants to merge 1 commit into from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 23, 2021

This removes the dependence on inlining for performance, so we also
remove @inline, since it can harm performance.

@nanosoldier runbenchmarks("broadcast" || "sparse" || "array", vs=":master")

@vtjnash vtjnash requested a review from mbauman November 23, 2021 17:20
@mbauman
Copy link
Member

mbauman commented Nov 23, 2021

This looks like it updates and replaces #35675? Does it work around the problem of the mutable Ref?

vtjnash added a commit that referenced this pull request Nov 23, 2021
@vtjnash
make Some type a zero-dim broadcast container (e.g. a scalar)
59b8341 
Replaces #35778
Replaces #39184
Fixes #39151
Refs #35675
Refs #43200
@vtjnash
Copy link
Member Author

vtjnash commented Nov 23, 2021

Ah, I had forgotten #35675 existed. I think we need to stop preventing progress on this bug. We seen these @inline substantially harm performance in many cases, but in a few cases lead to trivial allocations, which should typically not be considered a blocking issue.

@vtjnash
Copy link
Member Author

vtjnash commented Nov 23, 2021

I can put up a PR to add support in broadcast for Some (currently an error). And later we can also merge the PR that adds support for FillArray (currently not yet defined).

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

N5N3 pushed a commit to N5N3/julia that referenced this pull request Nov 30, 2021
@vtjnash @N5N3
broadcast: disable nospecialize logic for outer method signature
49e6d72 
This removes the dependence on inlining for performance, so we also
remove `@inline`, since it can harm performance.

make Some type a zero-dim broadcast container (e.g. a scalar)

Replaces JuliaLang#35778
Replaces JuliaLang#39184
Fixes JuliaLang#39151
Refs JuliaLang#35675
Refs JuliaLang#43200
N5N3 pushed a commit to N5N3/julia that referenced this pull request Dec 1, 2021
@vtjnash @N5N3
broadcast: disable nospecialize logic for outer method signature
ddf421a 
This removes the dependence on inlining for performance, so we also
remove `@inline`, since it can harm performance.

make Some type a zero-dim broadcast container (e.g. a scalar)

Replaces JuliaLang#35778
Replaces JuliaLang#39184
Fixes JuliaLang#39151
Refs JuliaLang#35675
Refs JuliaLang#43200
N5N3 pushed a commit to N5N3/julia that referenced this pull request Dec 1, 2021
@vtjnash @N5N3
broadcast: disable nospecialize logic for outer method signature
06a16b8 
This removes the dependence on inlining for performance, so we also
remove `@inline`, since it can harm performance.

make Some type a zero-dim broadcast container (e.g. a scalar)

Replaces JuliaLang#35778
Replaces JuliaLang#39184
Fixes JuliaLang#39151
Refs JuliaLang#35675
Refs JuliaLang#43200
@vtjnash
broadcast: disable nospecialize logic for outer method signature
e902d8b 
This removes the dependence on inlining for performance, so we also
remove `@inline`, since it can harm performance.
@vtjnash vtjnash force-pushed the jn/better-broadcast-inlining branch from 479c9a3 to e902d8b Compare September 7, 2022 19:40
@giordano giordano added performance Must go faster broadcast Applying a function over a collection labels Feb 14, 2023
@KristofferC
Copy link
Member

KristofferC commented May 21, 2024 •
edited
Loading

bump

Edit: Ok, I read #35675 (comment) etc now...

@KristofferC KristofferC mentioned this pull request May 21, 2024
Open
@vtjnash
Copy link
Member Author

vtjnash commented May 21, 2024

It would be good to merge, so that folks had to actually merge the fix for that problem and not ignore them any longer

@mbauman
Copy link
Member

mbauman commented May 21, 2024

I mean, I'm all for this, but we should know what the tradeoffs are and make an intentional decision that the good outweighs the bad.

@KristofferC do you have a branch with conflicts resolved against which we can run nanosoldier again? 

@oscardssmith oscardssmith added the triage This should be discussed on a triage call label May 21, 2024
@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbauman mbauman Awaiting requested review from mbauman

Assignees
No one assigned
Labels
broadcast Applying a function over a collection performance Must go faster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vtjnash @mbauman @nanosoldier @KristofferC @giordano @oscardssmith @LilithHafner