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

Optimize the Julia code #5

Merged
merged 1 commit into from May 22, 2021
Merged

Optimize the Julia code #5

merged 1 commit into from May 22, 2021

Conversation

ChrisRackauckas
Copy link
Contributor

The Julia code was very unoptimized so I did a few changes. BTW, why is the a_list passed in as a Vector{Any}?

The Julia code was very unoptimized so I did a few changes. BTW, why is the `a_list` passed in as a `Vector{Any}`?
@ChrisRackauckas
Copy link
Contributor Author

BTW, welcome to the community. If you need some help getting your code optimized, do check out https://discourse.julialang.org/ and https://julialang.org/slack/ . The #helpdesk and #performance-helpdesk channels in the Slack may be particularly helpful, and feel free to ping if you need anything!

@@ -16,10 +16,10 @@ end

function make_list(a_list::Vector{Any})
convert(Vector{Vector{Float64}}, a_list)

Choose a reason for hiding this comment

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

This line doesn't do anything, I think?

Choose a reason for hiding this comment

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

And hence make_list([]) returns a Vector{Any} which iterate_list won't accept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does something if a_list::Vector{Any}, which is why I'm a bit confused by what is passed in.

Copy link

@giordano giordano Apr 19, 2021

Choose a reason for hiding this comment

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

convert isn't in-place (you can't change the type of a container anyway) and the result isn't assigned to anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code that is calling this isn't part of the repo, otherwise I'd change whatever is doing a_list = [] to a_list = Vector{Float64}() which would presumably solve this.

Copy link
Owner

Choose a reason for hiding this comment

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

convert isn't in-place (you can't change the type of a container anyway) and the result isn't assigned to anything.

Thanks, i didn't know... But with that line iterate_list accepts Vector{Vector{Float64}}, without it only accepts Vector{Any}

BTW, the Vector{Any} is decided by pyjulia+PyCall. It's not the best, but it's so for now... I opened an issue about this:

JuliaPy/pyjulia#441

Choose a reason for hiding this comment

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

But with that line iterate_list accepts Vector{Vector{Float64}}, without it only accepts Vector{Any}

As I already said above, convert isn't in-place (and it can't possibly be) and you aren't assigning its result to anything: this line is doing work that isn't effectively used anywhere. It has no practical effects, besides wasting CPU cycles.

for j in 1:10^4
push!(new_list, 0.01)
@inbounds for i in 1:10^4
new_list = zeros(10^4)

Choose a reason for hiding this comment

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

Does this need to be initialized to zero?

Choose a reason for hiding this comment

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

It does not, but is shorter to write.

If it's legal to allocate the entire array at once, not push! every time, then you could also replace the entire function with something like this:

make_list() = [fill(0.1, 10^4) for _ in 1:10^4];

I guess it would be cheating to use fill(fill(0.1, 10^4), 10^4), although that's much faster!

Copy link
Owner

Choose a reason for hiding this comment

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

I'm going to change the test to avoid such suggestions. The problem is about lists, not arrays.

Choose a reason for hiding this comment

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

OK. Note that Julia doesn't really have this distinction, the length (but not the type) of any one-dimensional array can be mutated. The memory allocated need not match the length used, so that sometimes push! just writes to the next address, but when full, it re-allocates new memory (IIRC) twice as big. So a more modest change is to hint at the memory required:

               new_list = Float64[]
               sizehint!(new_list, 10^4)

This is roughly twice as fast, and 1/3 the memory.

The first line is just tidying up, as new_list::Vector{Float64} = [] allocates a new Vector{Any}, and then sees that it needs to convert it to Vector{Float64} to attach the name new_list.

f1() = new_list = Float64[];
f2() = new_list::Vector{Float64} = [];
@time f1()  # 1 allocation: 80 bytes
@time f2()  # 2 allocations: 160 bytes

@00sapo 00sapo merged commit 71ea71e into 00sapo:main May 22, 2021
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.

None yet

5 participants