Skip to content

fix: read request body correctly#134

Merged
savonarola merged 1 commit intoemqx:masterfrom
savonarola:20250722-fix-read-body
Jul 22, 2025
Merged

fix: read request body correctly#134
savonarola merged 1 commit intoemqx:masterfrom
savonarola:20250722-fix-read-body

Conversation

@savonarola
Copy link
Copy Markdown
Contributor

No description provided.

Comment on lines +114 to +115
{more, Bin, NewRequest} -> binary_decoder(NewRequest, [Bin | Acc]);
{ok, Bin, DoneRequest} -> {ok, iolist_to_binary(lists:reverse([Bin | Acc])), DoneRequest}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just wanted to chime in with previous experience.

Deep improper lists may show slightly better performance, likely due to less GC pressure. I.e.

Suggested change
{more, Bin, NewRequest} -> binary_decoder(NewRequest, [Bin | Acc]);
{ok, Bin, DoneRequest} -> {ok, iolist_to_binary(lists:reverse([Bin | Acc])), DoneRequest}
{more, Bin, NewRequest} -> binary_decoder(NewRequest, [Acc | Bin]);
{ok, Bin, DoneRequest} -> {ok, iolist_to_binary(Acc)), DoneRequest}
GC Pressure @ 10k chunks of 400 bytes each
emqx_binacc_bench:accum_deep(Es)         emqx_binacc_bench:accum_reverse(Es, [])
                                         
minor_gcs:                  100          184
minor_gcs_duration:         773          5247
major_gcs:                    0          0
major_gcs_duration:           0          0
heap_reclaimed:         2100919          2907062
offheap_bin_reclaimed: 50002500          50002475
stack_min:                    5          4
stack_max:                    5          6

Admittedly, this is pretty hard to prove in practice: simple synthetic microbenchmarks fluctuate between ~40% improvement in runtime to slight negative effect with around 10-15% improvement on average, depending on the choice of parameters. Which probably hints at memory allocation machinery complexity. So take it with a grain of salt.

eministat @ 10k chunks 400 bytes
x accrev / Fixed List 10k
+ deep / Fixed List 10k
+--------------------------------------------------------------------------+
|++*********xxxxx         xx xxx xxxxx xxxx xxxxx   x                     x|
|+++****** *xx  x            x x x xxx xx x xxxx                           |
|+++**+*** x x                 x x x x  x x  x                             |
| +++*+*** x                   x x   x  x    x                             |
|   +*+**x                     x x   x  x                                  |
|   +*+**x                       x   x                                     |
|   +++**x                                                                 |
|   +++*+                                                                  |
|   +++++                                                                  |
|   +++++                                                                  |
|   +++++                                                                  |
|   + +++                                                                  |
|   + ++                                                                   |
|     ++                                                                   |
|     ++                                                                   |
|     ++                                                                   |
|     ++                                                                   |
|     ++                                                                   |
|     ++                                                                   |
|     ++                                                                   |
|     ++                                                                   |
|     ++                                                                   |
|     +                                                                    |
|     +                                                                    |
|       |_______________A__M____________|                                  |
|   |_A_|                                                                  |
+--------------------------------------------------------------------------+
Dataset: x N=100 CI=95.0000
Statistic     Value     [         Bias] (Bootstrapped LB‥UB)
Min:            1859.00
1st Qu.         2335.00
Median:         4175.00
3rd Qu.         5221.00
Max:            8858.00
Average:        3908.50 [     0.290371] (      3597.91 ‥       4228.45)
Std. Dev:       1602.88 [     -9.82188] (      1489.09 ‥       1882.22)

Outliers: 0/0 = 0 (μ=3908.79, σ=1593.06)
	Outlier variance:      0.989089 (severe, the data set is probably unusable)

------

Dataset: + N=100 CI=95.0000
Statistic     Value     [         Bias] (Bootstrapped LB‥UB)
Min:            1620.00
1st Qu.         1978.00
Median:         2137.00
3rd Qu.         2248.00
Max:            2566.00
Average:        2110.53 [    -0.248106] (      2071.25 ‥       2149.37)
Std. Dev:       196.815 [     -1.41853] (      171.619 ‥       227.719)

Outliers: 0/0 = 0 (μ=2110.28, σ=195.397)
	Outlier variance:      0.769069 (severe, the data set is probably unusable)

Difference at 95.0% confidence
	-1797.97 ± 316.524
	-46.0015% ± 8.09836%
	(Student's t, pooled s = 1141.92)
------

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insights, eministat visualizes significant difference, I changed the implementation.

However, this code is probably almost never called in practice 😅

@savonarola savonarola force-pushed the 20250722-fix-read-body branch from 35dae3f to bd414bc Compare July 22, 2025 12:59
@savonarola savonarola merged commit 2ca1dfd into emqx:master Jul 22, 2025
2 checks passed
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