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

Remove unnecessary lock in isVisibleNoLock check #4623

Merged
merged 2 commits into from
Dec 11, 2024
Merged

Conversation

ray6080
Copy link
Contributor

@ray6080 ray6080 commented Dec 11, 2024

Description

Remove the unnecessary lock in NodeTable::isVisibleNoLock, which was introduced in #4467.

The lock caused large regression on COPY REL performance.
On a Mac Studio with M1 Max Pro and 64GB RAM, copying of ldbc-1000 knows table is improved from 115862ms to 41669ms.

@ray6080 ray6080 requested review from royi-luo and removed request for benjaminwinger December 11, 2024 08:40
Copy link

Benchmark Result

Master commit hash: ff64a4dfded73a6f57b6a9cd20bc10c113bef3b9
Branch commit hash: a6320bdc7ffdc97ab99d5d38dae00f5e8b227a6d

Query Group Query Name Mean Time - Commit (ms) Mean Time - Master (ms) Diff
aggregation q24 645.34 642.87 2.47 (0.38%)
aggregation q28 11856.21 11172.04 684.17 (6.12%)
filter q14 126.66 125.47 1.20 (0.95%)
filter q15 128.54 121.72 6.82 (5.60%)
filter q16 303.71 307.38 -3.67 (-1.19%)
filter q17 448.19 450.70 -2.51 (-0.56%)
filter q18 1925.65 1918.24 7.41 (0.39%)
filter zonemap-node 86.90 86.10 0.80 (0.93%)
filter zonemap-node-lhs-cast 86.69 89.55 -2.86 (-3.19%)
filter zonemap-rel 5677.69 5748.51 -70.82 (-1.23%)
fixed_size_expr_evaluator q07 572.63 585.07 -12.45 (-2.13%)
fixed_size_expr_evaluator q08 803.31 805.49 -2.18 (-0.27%)
fixed_size_expr_evaluator q09 802.48 815.72 -13.25 (-1.62%)
fixed_size_expr_evaluator q10 237.54 242.55 -5.01 (-2.06%)
fixed_size_expr_evaluator q11 230.72 235.27 -4.54 (-1.93%)
fixed_size_expr_evaluator q12 227.56 230.07 -2.51 (-1.09%)
fixed_size_expr_evaluator q13 1461.83 1463.23 -1.41 (-0.10%)
fixed_size_seq_scan q23 111.21 108.73 2.48 (2.28%)
join q29 594.14 624.57 -30.42 (-4.87%)
join q30 1514.85 1608.86 -94.02 (-5.84%)
join q31 7.66 5.41 2.24 (41.44%)
ldbc_snb_ic q35 2630.04 2673.10 -43.07 (-1.61%)
ldbc_snb_ic q36 568.41 542.79 25.62 (4.72%)
ldbc_snb_is q32 3.36 3.44 -0.07 (-2.14%)
ldbc_snb_is q33 11.74 10.46 1.28 (12.28%)
ldbc_snb_is q34 1.20 1.24 -0.04 (-3.26%)
multi-rel multi-rel-large-scan 1228.47 1221.01 7.46 (0.61%)
multi-rel multi-rel-lookup 28.97 5.40 23.57 (436.25%)
multi-rel multi-rel-small-scan 91.05 98.75 -7.70 (-7.79%)
order_by q25 129.07 131.68 -2.61 (-1.98%)
order_by q26 445.64 462.06 -16.42 (-3.55%)
order_by q27 1462.71 1453.24 9.47 (0.65%)
recursive_join recursive-join-bidirection 310.34 301.56 8.78 (2.91%)
recursive_join recursive-join-dense 7309.25 7004.87 304.38 (4.35%)
recursive_join recursive-join-path 23988.78 23728.10 260.68 (1.10%)
recursive_join recursive-join-sparse 14377.65 14674.73 -297.08 (-2.02%)
recursive_join recursive-join-trail 7295.61 7370.70 -75.09 (-1.02%)
scan_after_filter q01 173.09 172.32 0.77 (0.45%)
scan_after_filter q02 159.00 155.98 3.02 (1.93%)
shortest_path_ldbc100 q37 90.98 87.51 3.47 (3.97%)
shortest_path_ldbc100 q38 320.18 362.28 -42.11 (-11.62%)
shortest_path_ldbc100 q39 66.13 63.22 2.90 (4.59%)
shortest_path_ldbc100 q40 459.25 425.15 34.10 (8.02%)
var_size_expr_evaluator q03 2066.96 2078.32 -11.36 (-0.55%)
var_size_expr_evaluator q04 2240.04 2261.14 -21.09 (-0.93%)
var_size_expr_evaluator q05 2684.49 2686.41 -1.93 (-0.07%)
var_size_expr_evaluator q06 1339.52 1346.40 -6.88 (-0.51%)
var_size_seq_scan q19 1446.40 1460.29 -13.89 (-0.95%)
var_size_seq_scan q20 2709.66 2719.58 -9.92 (-0.36%)
var_size_seq_scan q21 2276.19 2345.01 -68.83 (-2.93%)
var_size_seq_scan q22 124.11 126.88 -2.77 (-2.18%)

@ray6080 ray6080 merged commit 5e75977 into master Dec 11, 2024
@ray6080 ray6080 deleted the is-visible-no-lock branch December 11, 2024 09:26
ray6080 added a commit that referenced this pull request Dec 17, 2024
* avoid lock in isVisibleNoLock check

* Run clang-format

---------

Co-authored-by: CI Bot <[email protected]>
ray6080 added a commit that referenced this pull request Dec 18, 2024
* avoid lock in isVisibleNoLock check

* Run clang-format

---------

Co-authored-by: CI Bot <[email protected]>
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