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.

Sign up for GitHub

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

Jump to bottom

Return Error From Postgres Float Deserialization #3944

Merged
merged 9 commits into from
May 24, 2024
Merged

Return Error From Postgres Float Deserialization #3944

merged 9 commits into from
May 24, 2024

Conversation

JonBoyleCoding
Copy link
Contributor

I had an error where I was getting the debug assertion panic on a table and I had no idea what field was causing the issue. Here's a modification I made to ensure that any deserialization errors get passed up the chain with the field name without panicking in order to properly diagnose where my issue is happening.

I don't know if his is wanted or not, but thought I'd make the PR in case it is. Happy to do more modifications if necessary.

@weiznich weiznich requested a review from a team February 23, 2024 15:12
weiznich
weiznich requested changes Feb 23, 2024
View reviewed changes
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR 👍. Overall I'm happy with the suggestion to return a more useful error in this situation. I've made some suggestions how we could improve this in a more general way. With these suggestions applied + a test case for the new error I would be happy to merge this.

It would be great if that could also adjust the code in these impl: https://github.com/diesel-rs/diesel/blob/bc263afa1765f650d9c4bf36c40536ff95b9cade/diesel/src/pg/types/integers.rs#L45-L63 ( (although that's not necessary for merging this PR)

There is a bit more discussion on this topic in #3868

diesel/src/deserialize.rs Outdated Show resolved Hide resolved
diesel/src/pg/types/floats/mod.rs Outdated Show resolved Hide resolved
@JonBoyleCoding
Copy link
Contributor Author

Converted the code to a specific error message as you requested - had to do some weird stuff to coerce the compiler to build correctly with OK(...?) (the compiler suggested it).

@JonBoyleCoding
Copy link
Contributor Author

If the change is ok I can look into writing a test case.

weiznich
weiznich reviewed Feb 26, 2024
View reviewed changes
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for updating the code. I've noticed an issue that I missed while proposing this solution. I've left a comment how to fix that. Otherwise this approach looks good now.

diesel/src/deserialize.rs Outdated Show resolved Hide resolved
diesel/src/result.rs Show resolved Hide resolved
weiznich
weiznich reviewed Feb 27, 2024
View reviewed changes
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

In terms of changes this looks fine now, so good work 👍

I would like to have a test for this before merging (put it into diesel_tests, should create a table, and query as the wrong type + check for this error) + this likely requires an entry in the changelog (See the Changelog.md file for this).

diesel/src/result.rs Show resolved Hide resolved
@weiznich weiznich added the run-benchmarks Used to indicate that github actions should run our benchmark suite label Mar 8, 2024
@JonBoyleCoding
Copy link
Contributor Author

Hi - sorry I haven't had a chance to work on this further. I saw that you have continued this on so thanks for that. I have been very busy at work and was unable to work on writing tests.

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024

🐰Bencher

ReportFri, March 8, 2024 at 09:50:47 UTC
Projectdiesel
Branchdiagnostics-branch-ee46bed4
Testbedubuntu-latest-sqlite
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)		Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)2763.300 (-0.78%)3128.222 (88.33%)
bench_insert/diesel/10✅ (view plot)6875.100 (+0.63%)7519.289 (91.43%)
bench_insert/diesel/100✅ (view plot)43954.000 (+0.43%)46746.727 (94.03%)
bench_insert/diesel/25✅ (view plot)13329.000 (+0.56%)14438.644 (92.31%)
bench_insert/diesel/50✅ (view plot)23991.000 (+1.83%)30403.102 (78.91%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)403130.000 (+0.68%)443792.107 (90.84%)
bench_medium_complex_query/diesel/1✅ (view plot)3410.700 (+1.46%)4142.767 (82.33%)
bench_medium_complex_query/diesel/10✅ (view plot)5548.400 (+1.35%)6648.362 (83.46%)
bench_medium_complex_query/diesel/100✅ (view plot)28613.000 (+1.47%)34801.683 (82.22%)
bench_medium_complex_query/diesel/1000✅ (view plot)248500.000 (+1.61%)307258.979 (80.88%)
bench_medium_complex_query/diesel/10000✅ (view plot)2432500.000 (+1.49%)2962745.796 (82.10%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)4750.500 (+0.37%)5011.155 (94.80%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)6919.200 (+0.26%)7190.281 (96.23%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)30151.000 (+1.59%)37173.778 (81.11%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)250050.000 (+1.46%)303819.307 (82.30%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)2438200.000 (+1.58%)3002703.250 (81.20%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)18499.000 (-0.22%)19183.229 (96.43%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)21005.000 (-0.49%)22753.585 (92.32%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)49810.000 (+0.59%)54136.865 (92.01%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)319160.000 (+0.03%)320351.564 (99.63%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)2978000.000 (-0.89%)3428239.620 (86.87%)
bench_trivial_query/diesel/1✅ (view plot)855.360 (+0.05%)862.137 (99.21%)
bench_trivial_query/diesel/10✅ (view plot)2846.800 (+0.80%)3182.672 (89.45%)
bench_trivial_query/diesel/100✅ (view plot)22983.000 (-0.52%)25010.345 (91.89%)
bench_trivial_query/diesel/1000✅ (view plot)218040.000 (-0.40%)232653.781 (93.72%)
bench_trivial_query/diesel/10000✅ (view plot)2128600.000 (-1.22%)2571237.075 (82.79%)
bench_trivial_query/diesel_boxed/1✅ (view plot)1441.900 (+0.10%)1463.497 (98.52%)
bench_trivial_query/diesel_boxed/10✅ (view plot)3407.200 (-0.91%)3934.310 (86.60%)
bench_trivial_query/diesel_boxed/100✅ (view plot)23554.000 (-1.44%)29348.829 (80.26%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)221140.000 (-0.25%)230516.472 (95.93%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)2140900.000 (-1.13%)2553126.894 (83.85%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)5463.600 (+0.38%)5771.172 (94.67%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)8039.400 (-0.39%)8570.733 (93.80%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)34002.000 (-0.39%)36274.316 (93.74%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)285020.000 (-0.12%)290933.091 (97.97%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)2741100.000 (-1.51%)3450670.883 (79.44%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024

🐰Bencher

ReportFri, March 8, 2024 at 09:50:47 UTC
Projectdiesel
Branchdiagnostics-branch-ee46bed4
Testbedubuntu-latest-mysql
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)		Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)1340400.000 (+1.56%)1646482.897 (81.41%)
bench_insert/diesel/10✅ (view plot)1093800.000 (-14.70%)4279266.430 (25.56%)
bench_insert/diesel/100✅ (view plot)2223300.000 (-6.20%)4705953.365 (47.24%)
bench_insert/diesel/25✅ (view plot)1388900.000 (+8.88%)3076451.931 (45.15%)
bench_insert/diesel/50✅ (view plot)1860000.000 (+0.45%)1985114.177 (93.70%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)16061000.000 (+1.31%)19159065.328 (83.83%)
bench_medium_complex_query/diesel/1✅ (view plot)113880.000 (+0.84%)127955.345 (89.00%)
bench_medium_complex_query/diesel/10✅ (view plot)148140.000 (+0.46%)158193.818 (93.64%)
bench_medium_complex_query/diesel/100✅ (view plot)213020.000 (-0.21%)220622.545 (96.55%)
bench_medium_complex_query/diesel/1000✅ (view plot)850760.000 (-0.08%)862586.181 (98.63%)
bench_medium_complex_query/diesel/10000✅ (view plot)7396000.000 (-0.26%)7720375.261 (95.80%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)116130.000 (+1.54%)142344.399 (81.58%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)151140.000 (+0.32%)158289.382 (95.48%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)219260.000 (+1.31%)261560.507 (83.83%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)851250.000 (-0.20%)880646.508 (96.66%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)7391400.000 (-0.41%)7901615.254 (93.54%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)193670.000 (+1.15%)226363.526 (85.56%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)228270.000 (+0.00%)228418.945 (99.93%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)306180.000 (+0.71%)338203.271 (90.53%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)967510.000 (-1.06%)1142875.375 (84.66%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)8066800.000 (+0.32%)8445121.439 (95.52%)
bench_trivial_query/diesel/1✅ (view plot)66361.000 (+0.57%)71953.902 (92.23%)
bench_trivial_query/diesel/10✅ (view plot)73626.000 (+1.91%)94158.130 (78.19%)
bench_trivial_query/diesel/100✅ (view plot)133740.000 (+0.59%)145432.218 (91.96%)
bench_trivial_query/diesel/1000✅ (view plot)675070.000 (-0.13%)689430.363 (97.92%)
bench_trivial_query/diesel/10000✅ (view plot)6011600.000 (+0.27%)6249167.990 (96.20%)
bench_trivial_query/diesel_boxed/1✅ (view plot)67335.000 (-0.62%)74405.367 (90.50%)
bench_trivial_query/diesel_boxed/10✅ (view plot)74198.000 (+0.63%)81146.305 (91.44%)
bench_trivial_query/diesel_boxed/100✅ (view plot)132190.000 (-0.06%)133626.036 (98.93%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)668360.000 (-0.46%)720733.089 (92.73%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)6008100.000 (-0.42%)6434687.257 (93.37%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)125810.000 (+0.43%)133778.581 (94.04%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)133210.000 (-0.06%)134561.564 (99.00%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)193190.000 (-0.58%)212196.363 (91.04%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)744160.000 (-0.07%)752607.272 (98.88%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)6133300.000 (+0.24%)6348526.173 (96.61%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024

🐰Bencher

ReportFri, March 8, 2024 at 09:50:49 UTC
Projectdiesel
Branchdiagnostics-branch-ee46bed4
Testbedubuntu-latest-postgres
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)		Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)223820.000 (-0.08%)226861.018 (98.66%)
bench_insert/diesel/10✅ (view plot)254190.000 (+0.31%)265956.690 (95.58%)
bench_insert/diesel/100✅ (view plot)594000.000 (-1.14%)709727.632 (83.69%)
bench_insert/diesel/25✅ (view plot)320850.000 (-0.20%)331493.563 (96.79%)
bench_insert/diesel/50✅ (view plot)471710.000 (+4.51%)774962.933 (60.87%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)5910500.000 (-3.46%)9492988.234 (62.26%)
bench_medium_complex_query/diesel/1✅ (view plot)81184.000 (+1.45%)98454.225 (82.46%)
bench_medium_complex_query/diesel/10✅ (view plot)88648.000 (+2.73%)123717.206 (71.65%)
bench_medium_complex_query/diesel/100✅ (view plot)116360.000 (-1.41%)144404.944 (80.58%)
bench_medium_complex_query/diesel/1000✅ (view plot)413960.000 (-1.79%)541598.286 (76.43%)
bench_medium_complex_query/diesel/10000✅ (view plot)3327000.000 (+0.91%)3775325.800 (88.12%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)83333.000 (+1.20%)98004.127 (85.03%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)90356.000 (-0.02%)90702.338 (99.62%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)122700.000 (+0.50%)131711.200 (93.16%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)419080.000 (-1.17%)503130.361 (83.29%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)3236900.000 (-0.45%)3485249.809 (92.87%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)238150.000 (-0.10%)242289.163 (98.29%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)239160.000 (-3.09%)367811.959 (65.02%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)279860.000 (-2.17%)384690.651 (72.75%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)614410.000 (-2.22%)849751.010 (72.30%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)3694600.000 (+0.64%)4042387.622 (91.40%)
bench_trivial_query/diesel/1✅ (view plot)64360.000 (+1.63%)79760.959 (80.69%)
bench_trivial_query/diesel/10✅ (view plot)67249.000 (-0.46%)72486.309 (92.77%)
bench_trivial_query/diesel/100✅ (view plot)97534.000 (+1.49%)118922.566 (82.01%)
bench_trivial_query/diesel/1000✅ (view plot)390790.000 (+0.95%)445378.507 (87.74%)
bench_trivial_query/diesel/10000✅ (view plot)3359700.000 (+1.07%)3891435.251 (86.34%)
bench_trivial_query/diesel_boxed/1✅ (view plot)66753.000 (+0.93%)75875.909 (87.98%)
bench_trivial_query/diesel_boxed/10✅ (view plot)69931.000 (-0.27%)73140.964 (95.61%)
bench_trivial_query/diesel_boxed/100✅ (view plot)101540.000 (+2.01%)131403.562 (77.27%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)383120.000 (+0.30%)400397.672 (95.68%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)3328600.000 (+0.44%)3546060.355 (93.87%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)152990.000 (+0.74%)169671.890 (90.17%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)154250.000 (+0.31%)161399.382 (95.57%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)186920.000 (-0.42%)200182.218 (93.37%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)488800.000 (-0.12%)498345.418 (98.08%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)3654600.000 (+0.49%)3920467.625 (93.22%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@weiznich
Copy link
Member

weiznich commented Mar 8, 2024

Hi - sorry I haven't had a chance to work on this further. I saw that you have continued this on so thanks for that. I have been very busy at work and was unable to work on writing tests.

No worries about that. It's great that submitted that PR at all.

@weiznich
Copy link
Member

To just leave a comment here: I've found a performance regression in this commit regarding to certain benchmarks while running it locally. I'm currently investigating options to workaround this regression.

Ten0
Ten0 approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@Ten0 Ten0 left a comment

Choose a reason for hiding this comment

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

LGTM :)

I'd benefit from this this soon as well as I will run another round of schema Int4 -> Int8 migration and I wouldn't want #3868 to occur again - any way I can help with the benchmark regression thing?

@github-actions GitHub Actions
Copy link

github-actions bot commented May 13, 2024 •
edited
Loading

🐰Bencher

ReportFri, May 24, 2024 at 09:18:50 UTC
Projectdiesel
Branchdiagnostics-branch-c19ace78
Testbedubuntu-latest-sqlite
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)		Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)2,803.20 (-0.15%)2,874.16 (97.53%)
bench_insert/diesel/10✅ (view plot)6,911.30 (-0.80%)7,853.17 (88.01%)
bench_insert/diesel/100✅ (view plot)44,235.00 (-1.01%)51,854.44 (85.31%)
bench_insert/diesel/25✅ (view plot)13,198.00 (-1.58%)16,788.09 (78.62%)
bench_insert/diesel/50✅ (view plot)23,607.00 (-0.92%)27,323.80 (86.40%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)412,180.00 (+1.15%)482,035.42 (85.51%)
bench_medium_complex_query/diesel/1✅ (view plot)3,337.80 (-0.54%)3,644.44 (91.59%)
bench_medium_complex_query/diesel/10✅ (view plot)5,516.40 (+0.20%)5,676.52 (97.18%)
bench_medium_complex_query/diesel/100✅ (view plot)28,411.00 (-0.59%)31,249.28 (90.92%)
bench_medium_complex_query/diesel/1000✅ (view plot)246,280.00 (-1.01%)288,938.73 (85.24%)
bench_medium_complex_query/diesel/10000✅ (view plot)2,411,400.00 (-1.11%)2,867,552.71 (84.09%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)4,758.80 (-1.49%)5,971.83 (79.69%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)6,848.50 (-1.42%)8,512.61 (80.45%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)30,051.00 (-0.57%)32,973.76 (91.14%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)248,880.00 (-1.04%)293,059.23 (84.92%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)2,409,200.00 (-1.22%)2,913,502.16 (82.69%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)18,925.00 (+0.96%)21,613.47 (87.56%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)21,401.00 (+0.63%)23,404.32 (91.44%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)50,193.00 (+0.64%)54,951.81 (91.34%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)322,230.00 (+0.25%)334,294.58 (96.39%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)3,022,700.00 (+0.60%)3,290,057.08 (91.87%)
bench_trivial_query/diesel/1✅ (view plot)893.86 (+0.28%)931.32 (95.98%)
bench_trivial_query/diesel/10✅ (view plot)2,850.60 (+0.52%)3,068.81 (92.89%)
bench_trivial_query/diesel/100✅ (view plot)23,084.00 (+0.08%)23,344.65 (98.88%)
bench_trivial_query/diesel/1000✅ (view plot)216,020.00 (-0.92%)249,893.56 (86.44%)
bench_trivial_query/diesel/10000✅ (view plot)2,149,800.00 (-0.70%)2,404,907.63 (89.39%)
bench_trivial_query/diesel_boxed/1✅ (view plot)1,452.90 (-0.45%)1,563.56 (92.92%)
bench_trivial_query/diesel_boxed/10✅ (view plot)3,379.10 (-0.22%)3,507.50 (96.34%)
bench_trivial_query/diesel_boxed/100✅ (view plot)23,708.00 (-0.06%)23,936.08 (99.05%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)219,520.00 (-0.65%)243,679.20 (90.09%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)2,146,600.00 (-0.72%)2,409,310.17 (89.10%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)5,533.80 (-0.77%)6,262.80 (88.36%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)8,220.70 (+0.04%)8,273.58 (99.36%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)34,015.00 (+0.56%)36,859.86 (92.28%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)278,800.00 (-0.45%)300,171.60 (92.88%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)2,684,500.00 (+0.25%)2,785,038.18 (96.39%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@github-actions GitHub Actions
Copy link

github-actions bot commented May 13, 2024 •
edited
Loading

🐰Bencher

ReportFri, May 24, 2024 at 09:18:44 UTC
Projectdiesel
Branchdiagnostics-branch-c19ace78
Testbedubuntu-latest-postgres
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)		Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)269,080.00 (-3.59%)438,447.81 (61.37%)
bench_insert/diesel/10✅ (view plot)295,280.00 (-7.93%)724,739.33 (40.74%)
bench_insert/diesel/100✅ (view plot)717,380.00 (+3.94%)1,122,586.09 (63.90%)
bench_insert/diesel/25✅ (view plot)365,940.00 (+4.57%)604,178.24 (60.57%)
bench_insert/diesel/50✅ (view plot)451,090.00 (-5.96%)933,682.67 (48.31%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)6,684,700.00 (-1.68%)8,619,970.11 (77.55%)
bench_medium_complex_query/diesel/1✅ (view plot)84,027.00 (+3.92%)131,220.37 (64.04%)
bench_medium_complex_query/diesel/10✅ (view plot)91,082.00 (+3.15%)132,481.39 (68.75%)
bench_medium_complex_query/diesel/100✅ (view plot)123,210.00 (+0.58%)133,710.65 (92.15%)
bench_medium_complex_query/diesel/1000✅ (view plot)425,970.00 (+2.32%)570,074.72 (74.72%)
bench_medium_complex_query/diesel/10000✅ (view plot)3,245,300.00 (-0.41%)3,473,376.36 (93.43%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)86,463.00 (+1.57%)106,347.22 (81.30%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)92,569.00 (+2.08%)120,622.88 (76.74%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)122,550.00 (+0.11%)124,486.29 (98.44%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)427,380.00 (-0.31%)449,934.22 (94.99%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)3,278,700.00 (+1.11%)3,815,648.34 (85.93%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)237,520.00 (-0.24%)247,149.89 (96.10%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)242,630.00 (-1.04%)285,542.14 (84.97%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)278,870.00 (-2.50%)399,581.52 (69.79%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)595,770.00 (-2.55%)859,071.48 (69.35%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)3,593,200.00 (-0.13%)3,669,225.45 (97.93%)
bench_trivial_query/diesel/1✅ (view plot)66,652.00 (-0.03%)66,981.44 (99.51%)
bench_trivial_query/diesel/10✅ (view plot)67,859.00 (-1.12%)80,825.56 (83.96%)
bench_trivial_query/diesel/100✅ (view plot)98,532.00 (+0.64%)107,855.99 (91.36%)
bench_trivial_query/diesel/1000✅ (view plot)388,880.00 (-0.79%)440,915.20 (88.20%)
bench_trivial_query/diesel/10000✅ (view plot)3,325,700.00 (+1.15%)3,888,713.80 (85.52%)
bench_trivial_query/diesel_boxed/1✅ (view plot)67,343.00 (+0.89%)76,205.25 (88.37%)
bench_trivial_query/diesel_boxed/10✅ (view plot)70,662.00 (+2.05%)91,842.04 (76.94%)
bench_trivial_query/diesel_boxed/100✅ (view plot)99,731.00 (+1.24%)117,865.11 (84.61%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)380,780.00 (+0.26%)395,227.71 (96.34%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)3,333,000.00 (+0.18%)3,423,112.00 (97.37%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)150,850.00 (-0.43%)161,746.98 (93.26%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)156,990.00 (-0.13%)160,453.38 (97.84%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)190,630.00 (+1.29%)226,898.22 (84.02%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)481,440.00 (+0.16%)493,057.74 (97.64%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)3,588,000.00 (+0.74%)3,978,237.07 (90.19%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@github-actions GitHub Actions
Copy link

github-actions bot commented May 13, 2024 •
edited
Loading

🐰Bencher

ReportFri, May 24, 2024 at 09:18:41 UTC
Projectdiesel
Branchdiagnostics-branch-c19ace78
Testbedubuntu-latest-mysql
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)		Latency Upper Boundary
nanoseconds (ns) | (%)
bench_insert/diesel/1✅ (view plot)816,540.00 (+0.25%)847,371.71 (96.36%)
bench_insert/diesel/10✅ (view plot)936,440.00 (-1.01%)1,098,036.32 (85.28%)
bench_insert/diesel/100✅ (view plot)1,733,700.00 (-4.88%)3,236,469.76 (53.57%)
bench_insert/diesel/25✅ (view plot)1,105,800.00 (-0.76%)1,248,558.90 (88.57%)
bench_insert/diesel/50✅ (view plot)1,341,500.00 (-0.43%)1,439,488.36 (93.19%)
bench_loading_associations_sequentially/diesel/bench_loading_associations_sequentially✅ (view plot)15,977,000.00 (-0.46%)17,218,749.05 (92.79%)
bench_medium_complex_query/diesel/1✅ (view plot)111,640.00 (+0.85%)125,640.87 (88.86%)
bench_medium_complex_query/diesel/10✅ (view plot)146,080.00 (+0.16%)149,505.75 (97.71%)
bench_medium_complex_query/diesel/100✅ (view plot)211,050.00 (+0.01%)211,273.42 (99.89%)
bench_medium_complex_query/diesel/1000✅ (view plot)841,720.00 (+0.46%)899,064.00 (93.62%)
bench_medium_complex_query/diesel/10000✅ (view plot)7,303,800.00 (-0.78%)8,271,857.42 (88.30%)
bench_medium_complex_query/diesel_boxed/1✅ (view plot)115,160.00 (+0.87%)129,980.07 (88.60%)
bench_medium_complex_query/diesel_boxed/10✅ (view plot)150,290.00 (+0.89%)170,099.74 (88.35%)
bench_medium_complex_query/diesel_boxed/100✅ (view plot)215,530.00 (+0.77%)240,031.53 (89.79%)
bench_medium_complex_query/diesel_boxed/1000✅ (view plot)842,850.00 (-0.05%)849,861.24 (99.18%)
bench_medium_complex_query/diesel_boxed/10000✅ (view plot)7,358,600.00 (-0.04%)7,411,817.82 (99.28%)
bench_medium_complex_query/diesel_queryable_by_name/1✅ (view plot)191,880.00 (+0.51%)206,253.24 (93.03%)
bench_medium_complex_query/diesel_queryable_by_name/10✅ (view plot)228,780.00 (+0.79%)255,515.71 (89.54%)
bench_medium_complex_query/diesel_queryable_by_name/100✅ (view plot)307,860.00 (+1.57%)378,683.56 (81.30%)
bench_medium_complex_query/diesel_queryable_by_name/1000✅ (view plot)978,360.00 (-0.23%)1,016,457.20 (96.25%)
bench_medium_complex_query/diesel_queryable_by_name/10000✅ (view plot)7,953,300.00 (-0.31%)8,376,508.35 (94.95%)
bench_trivial_query/diesel/1✅ (view plot)64,976.00 (-1.89%)86,094.18 (75.47%)
bench_trivial_query/diesel/10✅ (view plot)72,750.00 (+0.51%)78,208.85 (93.02%)
bench_trivial_query/diesel/100✅ (view plot)130,820.00 (-1.14%)156,246.29 (83.73%)
bench_trivial_query/diesel/1000✅ (view plot)682,180.00 (+0.17%)699,159.78 (97.57%)
bench_trivial_query/diesel/10000✅ (view plot)6,122,800.00 (+0.53%)6,606,127.98 (92.68%)
bench_trivial_query/diesel_boxed/1✅ (view plot)66,440.00 (+0.16%)68,011.37 (97.69%)
bench_trivial_query/diesel_boxed/10✅ (view plot)73,369.00 (-2.09%)99,783.62 (73.53%)
bench_trivial_query/diesel_boxed/100✅ (view plot)134,400.00 (-0.40%)143,523.05 (93.64%)
bench_trivial_query/diesel_boxed/1000✅ (view plot)680,820.00 (+0.47%)728,259.13 (93.49%)
bench_trivial_query/diesel_boxed/10000✅ (view plot)6,108,100.00 (+0.33%)6,408,225.08 (95.32%)
bench_trivial_query/diesel_queryable_by_name/1✅ (view plot)127,470.00 (-1.04%)150,024.22 (84.97%)
bench_trivial_query/diesel_queryable_by_name/10✅ (view plot)132,220.00 (-2.20%)182,565.74 (72.42%)
bench_trivial_query/diesel_queryable_by_name/100✅ (view plot)195,480.00 (+0.41%)207,321.16 (94.29%)
bench_trivial_query/diesel_queryable_by_name/1000✅ (view plot)754,180.00 (+0.39%)797,299.71 (94.59%)
bench_trivial_query/diesel_queryable_by_name/10000✅ (view plot)6,586,000.00 (+1.84%)8,360,685.02 (78.77%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@weiznich
Copy link
Member

@Ten0 I've pushed my local experimentation here: weiznich@5ef08d9

As far as I remember there was a ~5% instruction count regression for queryable_by_name queries on postgres. Some of the added attributes makes things slightly better. It likely just needs another round of local benchmarking to validate the results, I just run out of time back then and forgot about it.

@Ten0
Copy link
Member

Ten0 commented May 14, 2024 •
edited
Loading

@Ten0 I've pushed my local experimentation here: weiznich@5ef08d9

As far as I remember there was a ~5% instruction count regression for queryable_by_name queries on postgres. Some of the added attributes makes things slightly better. It likely just needs another round of local benchmarking to validate the results, I just run out of time back then and forgot about it.

Thanks! 😊
Do you have a quick pointer on how I could run the relevant instruction count benchmark(s)?

(Also if you have a quick recommendation on a good tool to look at the generated assembly that would be great, otherwise I'll find a way for this.)

@weiznich
Copy link
Member

Do you have a quick pointer on how I could run the relevant instruction count benchmark(s)?

You should be able to run the benchmarks via cargo bench --features "instruction_count Postgres" from the diesel_benches. It requires a Linux environment that has access to the performance counters (that can run perf).

There is also a "fast_run" feature that disables some of the benchmarks. Criterion also allows to store and use explicit baselines, which is useful for comparing to a specific version.

@weiznich
Copy link
Member

I've done some more benchmarks locally and I'm now quite certain that the regression in instruction count does not make the code measurably slower. At least perf diff did not show anything but noise after comparing the base branch with the change (including the optimizations).

@weiznich weiznich enabled auto-merge May 24, 2024 08:39
@weiznich
Minor optimisations
c19ace7 
* Mark the error branches as cold to help the compiler optimising for
the other case
* Mark some functions as inline

Both changes have been benchmarked and have shown minor performance improvements
@weiznich weiznich added this pull request to the merge queue May 24, 2024
Merged via the queue into diesel-rs:master with commit db6730c May 24, 2024
51 checks passed
@BrewTestBot BrewTestBot mentioned this pull request May 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weiznich weiznich weiznich approved these changes

@Ten0 Ten0 Ten0 approved these changes

Assignees
No one assigned
Labels
run-benchmarks Used to indicate that github actions should run our benchmark suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JonBoyleCoding @weiznich @Ten0