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

parse_json regression in String allocations #49249

Closed
vtjnash opened this issue Apr 4, 2023 · 10 comments · Fixed by #50929
Closed

parse_json regression in String allocations #49249

vtjnash opened this issue Apr 4, 2023 · 10 comments · Fixed by #50929
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Milestone
1.10

Comments

@vtjnash
Copy link
Member

vtjnash commented Apr 4, 2023

This commit (#48996) caused a regression in parse_json. Maybe related to #49241? @oscardssmith
@vtjnash vtjnash added performance Must go faster regression Regression in behavior compared to a previous version labels Apr 4, 2023
@oscardssmith
Copy link
Member

I don't see how it would be related to #49241 since #48996 doesn't affect 1.9...

@gbaraldi
Copy link
Member

gbaraldi commented Apr 4, 2023

It's not a 1.9 regression, it's a master regression 1.9 is fine.

@oscardssmith
Copy link
Member

is there a known mechanism by which improving effects could regress performance?

@KristofferC KristofferC added this to the 1.10 milestone Apr 4, 2023
@vtjnash
Copy link
Member Author

vtjnash commented Apr 4, 2023

#49241 is the only mechanism I know of currently, which is why I linked to that. Depends on what was changed in that commit though

@gbaraldi
Copy link
Member

gbaraldi commented Jul 13, 2023 •
edited
Loading

We went from
image
to
image

So we are boxing a lot of ints and tuples that we didn't before. The values themselves don't matter because they have a different rate.

@gbaraldi gbaraldi mentioned this issue Jul 13, 2023
Closed
@oscardssmith
Copy link
Member

Is this fixed by #50444?

@gbaraldi
Copy link
Member

No

@oscardssmith
Copy link
Member

Why not? was this issue not caused by #49241? If not, do we have another potential reason why improving effects would regress performance?

@gbaraldi
Copy link
Member

This is a separate issue that was just triggered by that change. But @aviatesk looked into it and I think it's something due to results not being inlineable and fun.

@oscardssmith oscardssmith mentioned this issue Aug 11, 2023
Closed
@JeffBezanson
Copy link
Member

duplicate of #50458.

@oscardssmith oscardssmith mentioned this issue Aug 15, 2023
Merged
oscardssmith added a commit that referenced this issue Aug 17, 2023
@oscardssmith
when widening tuple types in tmerge, only widen the complex parts (#5…
6e2e6d0 
…0929)

This is the part of #50927
required to fix #49249.
Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)`
would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION,
Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels
bad intuitively because giving the compiler more type information led it
to forget type information that it already knew about, and is especially
damaging because it led to unnecessary type instability when iterating
tuples with complex element types (because the iterator state should be
inferrable as an `Int` even if you have no idea what the tuple type is).

This is tagged for backport to 1.10 since it is a relatively unobtrusive
change and it fixes the string regression in a more proper way.
vtjnash pushed a commit to vtjnash/julia that referenced this issue Aug 18, 2023
@oscardssmith @vtjnash
when widening tuple types in tmerge, only widen the complex parts (Ju…
cd3671d 
…liaLang#50929)

This is the part of JuliaLang#50927
required to fix JuliaLang#49249.
Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)`
would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION,
Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels
bad intuitively because giving the compiler more type information led it
to forget type information that it already knew about, and is especially
damaging because it led to unnecessary type instability when iterating
tuples with complex element types (because the iterator state should be
inferrable as an `Int` even if you have no idea what the tuple type is).

This is tagged for backport to 1.10 since it is a relatively unobtrusive
change and it fixes the string regression in a more proper way.
vtjnash pushed a commit to vtjnash/julia that referenced this issue Aug 18, 2023
@oscardssmith @vtjnash
when widening tuple types in tmerge, only widen the complex parts (Ju…
eaf298d 
…liaLang#50929)

This is the part of JuliaLang#50927
required to fix JuliaLang#49249.
Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)`
would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION,
Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels
bad intuitively because giving the compiler more type information led it
to forget type information that it already knew about, and is especially
damaging because it led to unnecessary type instability when iterating
tuples with complex element types (because the iterator state should be
inferrable as an `Int` even if you have no idea what the tuple type is).

This is tagged for backport to 1.10 since it is a relatively unobtrusive
change and it fixes the string regression in a more proper way.
IanButterworth pushed a commit that referenced this issue Aug 19, 2023
@oscardssmith @IanButterworth
when widening tuple types in tmerge, only widen the complex parts (#5…
0737e80 
…0929)

This is the part of #50927
required to fix #49249.
Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)`
would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION,
Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels
bad intuitively because giving the compiler more type information led it
to forget type information that it already knew about, and is especially
damaging because it led to unnecessary type instability when iterating
tuples with complex element types (because the iterator state should be
inferrable as an `Int` even if you have no idea what the tuple type is).

This is tagged for backport to 1.10 since it is a relatively unobtrusive
change and it fixes the string regression in a more proper way.

(cherry picked from commit 6e2e6d0)
nalimilan pushed a commit that referenced this issue Nov 5, 2023
@oscardssmith @nalimilan
when widening tuple types in tmerge, only widen the complex parts (#5…
6a8e34e 
…0929)

This is the part of #50927
required to fix #49249.
Specifically, before this change `tmerge(Tuple{Any, Int}, Nothing)`
would be `Union{Nothing, Tuple{Any, Int}}` but `tmerge(Tuple{BIG_UNION,
Int}, Nothing)` would be `Union{Nothing, Tuple{Any, Any}}`. This feels
bad intuitively because giving the compiler more type information led it
to forget type information that it already knew about, and is especially
damaging because it led to unnecessary type instability when iterating
tuples with complex element types (because the iterator state should be
inferrable as an `Int` even if you have no idea what the tuple type is).

This is tagged for backport to 1.10 since it is a relatively unobtrusive
change and it fixes the string regression in a more proper way.

(cherry picked from commit 6e2e6d0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
performance Must go faster regression Regression in behavior compared to a previous version
Projects
None yet
Milestone
1.10
Development

Successfully merging a pull request may close this issue.

when widening tuple types in tmerge, only widen the complex parts oscardssmith/julia
5 participants
@vtjnash @JeffBezanson @KristofferC @oscardssmith @gbaraldi