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

Fix race condition in random engine #4827

Merged
merged 2 commits into from
Feb 2, 2025
Merged

Fix race condition in random engine #4827

merged 2 commits into from
Feb 2, 2025

Conversation

ray6080
Copy link
Contributor

@ray6080 ray6080 commented Feb 2, 2025

Description

pcg() is not thread safe. Fixed by locking for now.

The ideal way to fix this is to let each function holds its own local state, which holds a RandomEngine with a unique seed. This would require changes to ExpressionEvaluator and function interface, which is non-trivial. See issue #4828

Copy link

github-actions bot commented Feb 2, 2025

Benchmark Result

Master commit hash: 63f86462016b6628c65f7572b5920b3f3d2c88d6
Branch commit hash: de3208d30bee308a5bd59b300175b50d548b448b

Query Group Query Name Mean Time - Commit (ms) Mean Time - Master (ms) Diff
aggregation q24 682.65 687.12 -4.48 (-0.65%)
aggregation q28 6082.88 6145.00 -62.12 (-1.01%)
filter q14 117.85 126.90 -9.06 (-7.14%)
filter q15 117.35 128.04 -10.69 (-8.35%)
filter q16 299.23 305.87 -6.63 (-2.17%)
filter q17 439.58 445.82 -6.24 (-1.40%)
filter q18 1905.31 2002.36 -97.06 (-4.85%)
filter zonemap-node 80.47 91.12 -10.65 (-11.68%)
filter zonemap-node-lhs-cast 81.18 89.28 -8.10 (-9.08%)
filter zonemap-node-null 80.71 88.93 -8.22 (-9.24%)
filter zonemap-rel 5736.08 5661.52 74.57 (1.32%)
fixed_size_expr_evaluator q07 565.22 572.63 -7.41 (-1.29%)
fixed_size_expr_evaluator q08 794.59 805.96 -11.38 (-1.41%)
fixed_size_expr_evaluator q09 797.10 808.44 -11.35 (-1.40%)
fixed_size_expr_evaluator q10 230.77 239.42 -8.65 (-3.61%)
fixed_size_expr_evaluator q11 223.24 231.73 -8.48 (-3.66%)
fixed_size_expr_evaluator q12 218.99 227.25 -8.26 (-3.64%)
fixed_size_expr_evaluator q13 1468.49 1456.06 12.43 (0.85%)
fixed_size_seq_scan q23 103.41 118.76 -15.35 (-12.92%)
join q29 649.25 599.16 50.09 (8.36%)
join q30 10128.63 10525.18 -396.55 (-3.77%)
join q31 5.60 5.15 0.45 (8.73%)
join SelectiveTwoHopJoin 49.23 50.08 -0.85 (-1.70%)
ldbc_snb_ic q35 2515.21 2550.01 -34.80 (-1.36%)
ldbc_snb_ic q36 467.43 521.21 -53.78 (-10.32%)
ldbc_snb_is q32 6.28 6.30 -0.02 (-0.26%)
ldbc_snb_is q33 11.92 16.07 -4.16 (-25.87%)
ldbc_snb_is q34 1.40 1.52 -0.12 (-7.74%)
multi-rel multi-rel-large-scan 1322.12 1403.38 -81.25 (-5.79%)
multi-rel multi-rel-lookup 26.74 5.77 20.97 (363.36%)
multi-rel multi-rel-small-scan 99.97 97.01 2.97 (3.06%)
order_by q25 122.14 133.23 -11.09 (-8.32%)
order_by q26 450.67 463.69 -13.03 (-2.81%)
order_by q27 1443.40 1490.41 -47.00 (-3.15%)
recursive_join recursive-join-bidirection 308.78 307.66 1.12 (0.36%)
recursive_join recursive-join-dense 7352.31 7316.28 36.03 (0.49%)
recursive_join recursive-join-path 23496.87 23719.29 -222.42 (-0.94%)
recursive_join recursive-join-sparse 1058.75 1060.40 -1.65 (-0.16%)
recursive_join recursive-join-trail 7340.80 7292.91 47.89 (0.66%)
scan_after_filter q01 165.56 172.35 -6.79 (-3.94%)
scan_after_filter q02 148.49 157.00 -8.51 (-5.42%)
shortest_path_ldbc100 q37 82.15 97.48 -15.34 (-15.73%)
shortest_path_ldbc100 q38 368.98 380.84 -11.86 (-3.12%)
shortest_path_ldbc100 q39 63.16 51.04 12.12 (23.74%)
shortest_path_ldbc100 q40 458.20 386.09 72.11 (18.68%)
var_size_expr_evaluator q03 2049.56 2058.68 -9.11 (-0.44%)
var_size_expr_evaluator q04 2180.71 2233.61 -52.90 (-2.37%)
var_size_expr_evaluator q05 2635.23 2653.37 -18.13 (-0.68%)
var_size_expr_evaluator q06 1315.85 1359.95 -44.10 (-3.24%)
var_size_seq_scan q19 1427.68 1443.57 -15.89 (-1.10%)
var_size_seq_scan q20 2656.67 2589.86 66.81 (2.58%)
var_size_seq_scan q21 2262.73 2281.48 -18.75 (-0.82%)
var_size_seq_scan q22 123.49 127.14 -3.65 (-2.87%)

Copy link

codecov bot commented Feb 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.40%. Comparing base (69c6456) to head (b651d9d).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4827      +/-   ##
==========================================
+ Coverage   86.39%   86.40%   +0.01%     
==========================================
  Files        1399     1399              
  Lines       60434    60436       +2     
  Branches     7445     7443       -2     
==========================================
+ Hits        52209    52218       +9     
+ Misses       8059     8052       -7     
  Partials      166      166              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ray6080 ray6080 merged commit a575aec into master Feb 2, 2025
25 checks passed
@ray6080 ray6080 deleted the fix-uuid branch February 2, 2025 14:47
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