Skip to content

Use takeset to make loading zero-copy #2143

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

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

benjeffery
Copy link
Member

@benjeffery benjeffery commented Mar 3, 2022

Fixes #2131

Here is a comparison of this main and this branch's memory use (main first):
main
branch

Almost all of the remaining overhead is due to casting offsets up to 64bit, which uses 3x their size, with that off we get:
branch_nocast

@petrelharp
Copy link
Contributor

Awesome to see this moving. What's memtestmsp.py doing?

@benjeffery
Copy link
Member Author

Awesome to see this moving. What's memtestmsp.py doing?

import tskit
tables = tskit.TableCollection.load("big.trees")

This skips the extra copy for a tree sequence, which I'll deal with separately.

@petrelharp
Copy link
Contributor

Nice! So we're seeing a factor of 2 reduction in memory except for this offset casting thing?

@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #2143 (df4f635) into main (13499ff) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2143      +/-   ##
==========================================
- Coverage   93.30%   93.29%   -0.02%     
==========================================
  Files          27       27              
  Lines       25818    25895      +77     
  Branches     1163     1163              
==========================================
+ Hits        24090    24159      +69     
- Misses       1698     1706       +8     
  Partials       30       30              
Flag Coverage Δ
c-tests 92.21% <100.00%> (-0.02%) ⬇️
lwt-tests 89.05% <ø> (ø)
python-c-tests 72.09% <ø> (ø)
python-tests 98.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
c/tskit/tables.c 90.29% <100.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13499ff...df4f635. Read the comment docs.

@benjeffery
Copy link
Member Author

All the tests finally pass here. Some feedback on how I've solved the edge cases (e.g. if after out:) would be good.

@jeromekelleher
Copy link
Member

Let's get #2032 squared away first, will make it easier to see the new stuff here. I think we're pretty sold on the takeset idea basically working anyway, right?

@benjeffery
Copy link
Member Author

benjeffery commented Mar 8, 2022

Here's a clean example that we're getting what we expect in a very simple case - this is a tree sequence with only one row in the populations table: tc.populations.add_row(metadata=b"a"*10_000_000_000) and the memory usage when we load it to a TableCollection:
main_big_ragged
branch_big_ragged

@benjeffery
Copy link
Member Author

benjeffery commented Mar 8, 2022

Here is the case for an msprime generated file:
main_msp
branch_msp

Summarising events on the top (main) plot:

  • 0 python initialises and imports tskit.
  • 0.15 Loading starts, kastore allocs
  • ~0.19 kastore completes, calls to set_columns happen along with cast_offsets (0.22s blip is a cast buffer being freed)
  • 0.24 kastore closes and frees
    (time.sleep(0.1) happens here, added so we can see the settled value for the table collection)
  • 0.36 table collection freed
  • 0.4 python frees its stuff.

This branch is similar, but there are no set column allocs or kastore frees so it is shorter. You can still see the cast_offset allocations at the peak.

@jeromekelleher
Copy link
Member

jeromekelleher commented Mar 8, 2022

Brilliant! So all working as expected now then? (Analysis super helpful btw)

@benjeffery
Copy link
Member Author

So all working as expected now then?

Yes, I believe so! It has been useful to break all this down and check we understand it all. Basically, the 0.05seconds after 0.2 are snipped out and don't happen, so as well as the memory usage falling the load is quicker!

@benjeffery benjeffery changed the title Use take cols - WIP Use take cols to make loading zero-copy Mar 8, 2022
@benjeffery benjeffery changed the title Use take cols to make loading zero-copy Use takeset to make loading zero-copy Mar 8, 2022
@benjeffery
Copy link
Member Author

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Mar 8, 2022

rebase

✅ Branch has been successfully rebased

@benjeffery
Copy link
Member Author

Now that I've got a clean diff I can see a couple of errors, will fix today so don't review yet!

@benjeffery
Copy link
Member Author

@jeromekelleher Ok! Ready for review.

@benjeffery benjeffery force-pushed the use-take-cols branch 2 times, most recently from e847af2 to 1481630 Compare March 9, 2022 15:18
@benjeffery
Copy link
Member Author

@jeromekelleher I've made the changes we discussed - in doing so it revealed that read_table_ragged_col_t had two references to the offset array, one of which was direct and therefore non-NULLable by the caller of read_tables. I couldn't see why this was needed so have removed the second reference, it was added when we added support for 64bit.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Looks great, let's merge!

@benjeffery benjeffery added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Mar 9, 2022
@mergify mergify bot merged commit 7889fc0 into tskit-dev:main Mar 9, 2022
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Mar 9, 2022
@benjeffery benjeffery deleted the use-take-cols branch March 9, 2022 18:16
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.

Use column takeset and kastore GET_TAKES_OWNSHIP for zero-copy loading
3 participants