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

[async] Use llvm::SmallVector/llvm:SmallSet for latest state readers #1951

Merged
merged 1 commit into from
Oct 14, 2020

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Oct 14, 2020

This is to address #1936 (review). But TBH, I saw no performance difference... Still good to keep the data structures consistent, though.

I feel like it's fine to keep lastest_state_owners_ as is because it's not shown as a bottleneck, and it's not multi-leveled.

Related issue = #1936 (review)

[Click here for the format server]


@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #1951 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1951   +/-   ##
=======================================
  Coverage   43.65%   43.65%           
=======================================
  Files          45       45           
  Lines        6226     6226           
  Branches     1107     1107           
=======================================
  Hits         2718     2718           
  Misses       3334     3334           
  Partials      174      174           

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 b675bee...9349748. Read the comment docs.

@xumingkuan
Copy link
Contributor

xumingkuan commented Oct 14, 2020

async_mgpcg.py on kun:
Before:

473.875 ms 63.79%  rebuild_graph         [44 x  10.770 ms]
    403.036 ms 85.05%  insert_task           [208655 x   1.932 us]
         15.607 ms  3.87%  get_task_meta         [208655 x  74.796 ns]
        117.734 ms 29.21%  insert_task meta->input_states [583601 x 201.738 ns]
             50.695 ms 43.06%  insert_edge           [583601 x  86.866 ns]
             67.039 ms 56.94%  [unaccounted]
         80.928 ms 20.08%  insert_task meta->output_states [217798 x 371.572 ns]
             36.551 ms 45.17%  insert_edge           [522732 x  69.923 ns]
             44.377 ms 54.83%  [unaccounted]
         71.740 ms 17.80%  insert_task latest_state_readers_ [583601 x 122.927 ns]
        117.027 ms 29.04%  [unaccounted]
      1.597 ms  0.34%  reid_nodes            [44 x  36.305 us]
      1.465 ms  0.31%  reid_pending_nodes    [44 x  33.303 us]
     67.777 ms 14.30%  [unaccounted]

After:

446.719 ms 64.48%  rebuild_graph         [44 x  10.153 ms]
    379.415 ms 84.93%  insert_task           [208655 x   1.818 us]
         15.268 ms  4.02%  get_task_meta         [208655 x  73.172 ns]
        113.590 ms 29.94%  insert_task meta->input_states [583601 x 194.636 ns]
             48.635 ms 42.82%  insert_edge           [583601 x  83.336 ns]
             64.955 ms 57.18%  [unaccounted]
         82.662 ms 21.79%  insert_task meta->output_states [217798 x 379.535 ns]
             34.935 ms 42.26%  insert_edge           [522732 x  66.832 ns]
             47.727 ms 57.74%  [unaccounted]
         57.767 ms 15.23%  insert_task latest_state_readers_ [583601 x  98.984 ns] (time reduced!)
        110.128 ms 29.03%  [unaccounted]
      1.542 ms  0.35%  reid_nodes            [44 x  35.042 us]
      1.466 ms  0.33%  reid_pending_nodes    [44 x  33.324 us]
     64.296 ms 14.39%  [unaccounted]

If we change StateToNodesMap from llvm::SmallVector<std::pair<AsyncState, llvm::SmallSet<Node *, 8>>, 4> to llvm::SmallVector<std::pair<AsyncState, llvm::SmallSet<Node *, 8>>, 100>:

0.771  s 69.32%  rebuild_graph         [44 x  17.521 ms]
  636.297 ms 82.54%  insert_task           [208655 x   3.050 us]
       17.767 ms  2.79%  get_task_meta         [208655 x  85.151 ns]
      127.272 ms 20.00%  insert_task meta->input_states [583601 x 218.081 ns]
           50.515 ms 39.69%  insert_edge           [583601 x  86.558 ns]
           76.757 ms 60.31%  [unaccounted]
       86.888 ms 13.66%  insert_task meta->output_states [217798 x 398.940 ns]
           37.968 ms 43.70%  insert_edge           [522732 x  72.635 ns]
           48.920 ms 56.30%  [unaccounted]
       59.681 ms  9.38%  insert_task latest_state_readers_ [583601 x 102.263 ns]
      344.689 ms 54.17%  [unaccounted]
    8.575 ms  1.11%  reid_nodes            [44 x 194.880 us]
    2.866 ms  0.37%  reid_pending_nodes    [44 x  65.137 us]
  123.183 ms 15.98%  [unaccounted]

to llvm::SmallVector<std::pair<AsyncState, llvm::SmallSet<Node *, 16>>, 4>:

487.942 ms 65.44%  rebuild_graph         [44 x  11.090 ms]
    412.135 ms 84.46%  insert_task           [208655 x   1.975 us]
         15.255 ms  3.70%  get_task_meta         [208655 x  73.113 ns]
        122.649 ms 29.76%  insert_task meta->input_states [583601 x 210.160 ns]
             51.231 ms 41.77%  insert_edge           [583601 x  87.784 ns]
             71.419 ms 58.23%  [unaccounted]
         84.822 ms 20.58%  insert_task meta->output_states [217798 x 389.453 ns]
             36.829 ms 43.42%  insert_edge           [522732 x  70.454 ns]
             47.994 ms 56.58%  [unaccounted]
         61.341 ms 14.88%  insert_task latest_state_readers_ [583601 x 105.108 ns]
        128.067 ms 31.07%  [unaccounted]
      1.702 ms  0.35%  reid_nodes            [44 x  38.678 us]
      1.576 ms  0.32%  reid_pending_nodes    [44 x  35.817 us]
     72.529 ms 14.86%  [unaccounted]

Copy link
Contributor

@xumingkuan xumingkuan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@k-ye
Copy link
Member Author

k-ye commented Oct 14, 2020

Thanks for doing the benchmark! So it actually did accelerate things a bit...

@k-ye k-ye merged commit d3bf553 into taichi-dev:master Oct 14, 2020
@k-ye k-ye deleted the readers branch October 14, 2020 14:16
@yuanming-hu yuanming-hu mentioned this pull request Oct 14, 2020
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