ci: Try and avoid large pass-by-value instances #2623
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to 070ff3a. neqo-latest as client
neqo-latest as server
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-latest as client
neqo-latest as server
|
larseggert
force-pushed
the
fix-avoid-large-pass-by-value
branch
from
a6d27ec to
f6f50a3
Compare
|
It's no longer |
larseggert
force-pushed
the
fix-avoid-large-pass-by-value
branch
2 times, most recently
from
af6591d to
c9db8d0
Compare
This affects - `ecn::Count` - `SocketAddr` - `Http3StreamInfo` - `QpackSettings` - `IpAddr`
larseggert
force-pushed
the
fix-avoid-large-pass-by-value
branch
from
c9db8d0 to
a6702db
Compare
larseggert
requested review from
KershawChang,
martinthomson and
mxinden
as code owners
martinthomson
requested changes
May 12, 2025
There was a problem hiding this comment.
Was this conversion automated? Because there are a few patterns that I'm seeing that are concerning. I stopped half way, but hopefully the patterns are clear.
There's some real improvement possible here, but those problems are enough that this isn't a good idea as-is.
neqo-http3/src/features/extended_connect/webtransport_streams.rs
Outdated
Show resolved
Hide resolved
larseggert
added a commit
to larseggert/neqo
that referenced
this pull request
May 12, 2025
Follow-on to mozilla#2623.
larseggert
commented
May 12, 2025
| @@ -7,3 +7,5 @@ disallowed-macros = [ | |||
| allow-mixed-uninlined-format-args = false | |||
| allow-unwrap-in-tests = true | |||
| allow-dbg-in-tests = true | |||
| pass-by-value-size-limit = 16 | |||
| # avoid-breaking-exported-api = false # TODO: Enable this in a follow-on PR. | |||
There was a problem hiding this comment.
See larseggert#36
|
@martinthomson maybe I'm misunderstanding something about Rust and this lint. I thought avoiding pass-by-value inherently meant to pass a reference, even if the called function needs a value. If the called function needs a value, it dereferences, which does not involve a stack allocation (but still involved a memory copy). |
Benchmark resultsPerformance differences relative to e179d56. 1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: 💚 Performance has improved. time: [763.68 ms 793.32 ms 824.37 ms]
thrpt: [121.31 MiB/s 126.05 MiB/s 130.94 MiB/s]
change:
time: [−11.904% −7.0725% −1.5637%] (p = 0.01 < 0.05)
thrpt: [+1.5886% +7.6108% +13.512%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected. time: [315.04 ms 316.41 ms 317.79 ms]
thrpt: [31.468 Kelem/s 31.605 Kelem/s 31.742 Kelem/s]
change:
time: [−0.7238% −0.1910% +0.3621%] (p = 0.49 > 0.05)
thrpt: [−0.3608% +0.1914% +0.7290%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected. time: [34.442 ms 35.673 ms 36.912 ms]
thrpt: [27.092 elem/s 28.032 elem/s 29.035 elem/s]
change:
time: [−4.3758% +0.3371% +5.6276%] (p = 0.89 > 0.05)
thrpt: [−5.3277% −0.3360% +4.5760%]
1-conn/1-100mb-req/mtu-1504 (aka. Upload)/client: No change in performance detected. time: [7.2562 s 7.2700 s 7.2837 s]
thrpt: [13.729 MiB/s 13.755 MiB/s 13.781 MiB/s]
change:
time: [−0.4403% −0.1777% +0.0709%] (p = 0.18 > 0.05)
thrpt: [−0.0708% +0.1780% +0.4423%]
decode 4096 bytes, mask ff: No change in performance detected. time: [11.844 µs 11.853 µs 11.861 µs]
change: [−0.2345% −0.0302% +0.1849%] (p = 0.79 > 0.05)
decode 1048576 bytes, mask ff: No change in performance detected. time: [3.0309 ms 3.0332 ms 3.0356 ms]
change: [−0.0449% +0.0856% +0.2265%] (p = 0.20 > 0.05)
decode 4096 bytes, mask 7f: No change in performance detected. time: [19.999 µs 20.011 µs 20.022 µs]
change: [−0.0843% +0.0394% +0.1550%] (p = 0.53 > 0.05)
decode 1048576 bytes, mask 7f: No change in performance detected. time: [5.1216 ms 5.1240 ms 5.1262 ms]
change: [−0.3315% −0.0764% +0.1253%] (p = 0.55 > 0.05)
decode 4096 bytes, mask 3f: No change in performance detected. time: [8.2885 µs 8.2970 µs 8.3060 µs]
change: [−0.1941% +0.0081% +0.2282%] (p = 0.95 > 0.05)
decode 1048576 bytes, mask 3f: No change in performance detected. time: [2.1228 ms 2.1247 ms 2.1265 ms]
change: [−2.2534% −0.0754% +2.0491%] (p = 0.94 > 0.05)
1000 streams of 1 bytes/multistream: 💚 Performance has improved. time: [15.570 ns 15.613 ns 15.658 ns]
change: [−3.4906% −3.0740% −2.6564%] (p = 0.00 < 0.05)
1000 streams of 1000 bytes/multistream: 💚 Performance has improved. time: [15.709 ns 15.750 ns 15.792 ns]
change: [−3.0029% −2.5655% −2.1580%] (p = 0.00 < 0.05)
coalesce_acked_from_zero 1+1 entries: Change within noise threshold. time: [94.443 ns 94.528 ns 94.600 ns]
change: [−1.4413% −0.8275% −0.2154%] (p = 0.01 < 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [112.41 ns 112.53 ns 112.63 ns]
change: [−0.6507% −0.0843% +0.4502%] (p = 0.76 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [111.51 ns 111.70 ns 111.86 ns]
change: [−0.5427% +0.0296% +0.5935%] (p = 0.93 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [89.765 ns 90.202 ns 91.090 ns]
change: [−0.2369% +0.4611% +1.0893%] (p = 0.18 > 0.05)
RxStreamOrderer::inbound_frame(): Change within noise threshold. time: [110.61 ms 110.63 ms 110.65 ms]
change: [+0.2584% +0.2805% +0.3019%] (p = 0.00 < 0.05)
SentPackets::take_ranges: No change in performance detected. time: [9.6844 µs 9.7295 µs 9.7718 µs]
change: [−2.7648% +0.0309% +2.8024%] (p = 0.99 > 0.05)
transfer/pacing-false/varying-seeds: Change within noise threshold. time: [34.305 ms 34.331 ms 34.357 ms]
change: [+2.0471% +2.1630% +2.2723%] (p = 0.00 < 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold. time: [35.389 ms 35.439 ms 35.489 ms]
change: [+1.5690% +1.7672% +1.9788%] (p = 0.00 < 0.05)
transfer/pacing-false/same-seed: Change within noise threshold. time: [34.308 ms 34.330 ms 34.352 ms]
change: [+1.7672% +1.8677% +1.9653%] (p = 0.00 < 0.05)
transfer/pacing-true/same-seed: Change within noise threshold. time: [36.160 ms 36.189 ms 36.218 ms]
change: [+2.1170% +2.2402% +2.3655%] (p = 0.00 < 0.05)
Client/server transfer resultsPerformance differences relative to e179d56. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
|
|
Rust has the convention that the function signature should express what the function implementation needs. Example:
In the context of this pull request, I suggest only changing to a reference if the function only needs a reference. |
|
Ok, I will give this a do-over tomorrow. |
|
Max got it. It is my understanding that this moves things as much as that is possible: fn do_something(huge: ItsEnormous) -> ItsGargantuan {
ItsGargantuan { huge, ...Default::default() }
}That's not a firm guarantee, but the compiler can be somewhat smart about these things by using moves for these sorts of things. |
Signed-off-by: Lars Eggert <lars@eggert.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This affects
ecn::CountSocketAddrHttp3StreamInfoQpackSettingsIpAddr