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

types: fix cache computation #41935

Merged
merged 1 commit into from
Aug 23, 2021
Merged

types: fix cache computation #41935

merged 1 commit into from
Aug 23, 2021

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Aug 19, 2021

Need to compute cacheable after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by #36211
Fixes #41503

@vtjnash
types: fix cache computation
dfe3021 
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by #36211
Fixes #41503
@vtjnash vtjnash requested a review from martinholters August 19, 2021 18:18
@Sacha0
Copy link
Member

Sacha0 commented Aug 19, 2021

Thanks for working on this Jameson! Excited to test this out! :)

@martinholters
Copy link
Member

I remember being a bit puzzled about the cachable being piped through various function calls, stemming from different origins with slightly different computations rules. So this definitely improves clarity. 👍 And your reasoning also makes perfect sense. I'm bit hesitant to "officially" approve as I have no idea whether this change has any other implications. (Is there a good reason why is was structured the way it was before? Or did it just not really matter before #36211?)

martinholters
martinholters reviewed Aug 20, 2021
View reviewed changes
src/jltypes.c
@@ -1593,14 +1593,13 @@ jl_tupletype_t *jl_inst_arg_tuple_type(jl_value_t *arg1, jl_value_t **args, size
// `jl_typeof(ai)`, but that will require some redesign of the caching
// logic.
ai = (jl_value_t*)jl_wrap_Type(ai);
cacheable = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This one is beyond me. What case did this prevent from caching?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is a different way to compute the same thing

@JeffBezanson JeffBezanson added this to the 1.7 milestone Aug 20, 2021
@vtjnash vtjnash merged commit 292f1a9 into master Aug 23, 2021
@vtjnash vtjnash deleted the jn/41503b branch August 23, 2021 20:33
@Sacha0
Copy link
Member

Sacha0 commented Aug 24, 2021

🎉

@benlorenz
Copy link
Contributor

Can this be backported to 1.7 / get a backport 1.7 label please? (It does belong to the 1.7 milestone already but was merged only into master so far)
It should apply cleanly now that #41659 is already on the backports branch.

I also verified that this fixes the issue (jl_datatype_isinlinealloc segfault) we observed in Oscar.jl with the latest julia 1.7.

@Sacha0 Sacha0 mentioned this pull request Aug 25, 2021
Closed
KristofferC pushed a commit that referenced this pull request Aug 27, 2021
@vtjnash @KristofferC
types: fix cache computation (#41935)
96eab49 
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by #36211
Fixes #41503

(cherry picked from commit 292f1a9)
@KristofferC KristofferC mentioned this pull request Aug 27, 2021
Merged
63 tasks
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@vtjnash @LilithHafner
types: fix cache computation (JuliaLang#41935)
74a92e3 
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by JuliaLang#36211
Fixes JuliaLang#41503
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@vtjnash @LilithHafner
types: fix cache computation (JuliaLang#41935)
9519768 
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by JuliaLang#36211
Fixes JuliaLang#41503
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinholters martinholters martinholters left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.

Julia nightly throwing at jl_datatype_isinlinealloc
6 participants
@vtjnash @Sacha0 @martinholters @benlorenz @JeffBezanson @KristofferC