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

avoid deepcopy #6

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

Conversation

bicycle1885
Copy link
Collaborator

@bicycle1885 bicycle1885 commented Sep 8, 2022

The deepcopy function is notorious for its bad performance in many cases. This pull request implements a method of the copy function for Partition to avoid calling the deepcopy function. The following example shows an extreme case to speed up initialization of a large tree:

tree = sim_tree(n = 1_000_000)
@time internal_message_init!(tree, GaussianPartition())
# main
kenta@KS-MBP ~/w/MolecularEvolution (opt-simtree)> julia example.jl 
 12.008125 seconds (50.00 M allocations: 3.178 GiB, 6.99% gc time, 0.01% compilation time)

# PR
kenta@KS-MBP ~/w/MolecularEvolution (copy-message)> julia example.jl
  1.121317 seconds (20.00 M allocations: 1.032 GiB)

Internal constructors of discrete partition types are moved out of the struct scopes because it eliminates the default constructor, which is needed to make the generic copy method work for all partition types.

NOTE: There are more deepcopy calls that can be eliminated, but I want to keep each pull request as small as possible. Follow-up pull requests will eliminate more deepcopys.

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2022

Codecov Report

Merging #6 (fe19d26) into main (3193086) will increase coverage by 0.24%.
The diff coverage is 25.71%.

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   32.32%   32.56%   +0.24%     
==========================================
  Files          28       28              
  Lines        1915     1916       +1     
==========================================
+ Hits          619      624       +5     
+ Misses       1296     1292       -4     
Impacted Files Coverage Δ
src/models/discrete_models/discrete_partitions.jl 51.42% <10.34%> (+1.42%) ⬆️
src/MolecularEvolution.jl 100.00% <100.00%> (ø)
src/core/nodes/FelNode.jl 18.00% <100.00%> (ø)
src/models/continuous_models/gaussian_partition.jl 61.53% <0.00%> (+3.84%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@murrellb
Copy link
Member

murrellb commented Sep 8, 2022

(as discussed) To handle cases where Partitions have more complex structure, and to avoid constraints on new type definitions (eg. requiring keeping the default constructors), we're going to instead go with our own copypartition function, which defaults to deepcopy for generic Partitions, but can be specialised for performance benefits for specific Partition subtypes.

@murrellb
Copy link
Member

murrellb commented Sep 8, 2022

Note: we must make sure the style and semantics with this and copy_partition_to! are consistent.

@nossleinad nossleinad mentioned this pull request Apr 9, 2024
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.

3 participants