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

Eliminating dynamic dispatch performance bottleneck #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goerch
Copy link

@goerch goerch commented Oct 21, 2021

Adding specializations to Base.getproperty and Base.setproperty to eliminate dynamic dispatch on Julia 1.7.0-rc1

Adding specializations to Base.getproperty and Base.setproperty to eliminate dynamic dispatch on Julia 1.7.0-rc1
@krynju
Copy link
Owner

krynju commented Oct 22, 2021

Will wait on resolution/RCA on this JuliaLang/julia#42754 before merging.
This doesn't look like type piracy to me since all of this is passed through a method defined for an Union{Node,Nothing} type, but maybe this can get addressed directly in julia instead

@goerch
Copy link
Author

goerch commented Oct 22, 2021

Only method mentioned was Base.setproperty!(::Nothing) and we could live with or without it probably. But waiting on the resolution seems reasonable too.

@krynju
Copy link
Owner

krynju commented Oct 22, 2021

Yeah, but you don't define such a method here, the _setproperty is a method only accessible through the union definition, so it won't replace the default

Unless I'm overlooking something this looks good to me

@goerch
Copy link
Author

goerch commented Oct 22, 2021

Edit: DataStructures.jl uses

@test [] == detect_ambiguities(Base, Core, DataStructures)

to detect type piracy.

@krynju
Copy link
Owner

krynju commented Oct 27, 2021

@goerch JuliaLang/julia#42754 was merged, I guess that is included in the nightly build already

It doesn't really affect the benchmarks included in this repo (but I'm not sure whether they're 100% correct)

Let me know if it helps with your benchmarks and whether we still need this merged

@goerch
Copy link
Author

goerch commented Oct 28, 2021

Let me know if it helps with your benchmarks and whether we still need this merged

1.6.3 and 1.8.0-DEV now only show marginal improvements, 1.7.0-rc2 is still awaiting the patch. I committed a simplified version of the needed specialization.

@krynju
Copy link
Owner

krynju commented Oct 28, 2021

Ok, so what you're saying is that this specialization still yields some noticeable improvements on both 1.6 and master?
Is there any difference in code generated with and without it?

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