1965246 - neqo_glue::parse_headers spends a lot of time deserializing headers

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

parse_headers currently takes 22% of the socket thread CPU in this profile https://share.firefox.dev/44zwMQF

This is entirely overhead. Instead of serializing the headers into mozilla::net::Http3Stream::mFlatHttpRequestHeaders and then splitting that string into an array of strings, mozilla::net::Http3Stream::GetHeadersString and mozilla::net::Http3WebTransportSession::ConsumeHeaders can dirrectly construct and append the Header::new object to the Vec<Header> and then pass that vec to the neqo_http3conn_fetch call.

Comment 1

[Max Inden]

This should be a quick fix:

diff --git a/netwerk/socket/neqo_glue/src/lib.rs b/netwerk/socket/neqo_glue/src/lib.rs
index 21e82b920d9c..aaa19737afb3 100644
--- a/netwerk/socket/neqo_glue/src/lib.rs
+++ b/netwerk/socket/neqo_glue/src/lib.rs
@@ -958,13 +1078,13 @@ fn parse_headers(headers: &nsACString) -> Result<Vec<Header>, nsresult> {
                 if elem.is_empty() {
                     continue;
                 }
-                let hdr_str: Vec<_> = elem.splitn(2, ':').collect();
-                let name = hdr_str[0].trim().to_lowercase();
+                let mut hdr_str= elem.splitn(2, ':');
+                let name = hdr_str.next().unwrap().trim().to_lowercase();
                 if is_excluded_header(&name) {
                     continue;
                 }
-                let value = if hdr_str.len() > 1 {
-                    String::from(hdr_str[1].trim())
+                let value = if let Some(value) =  hdr_str.next(){
+                    String::from(value.trim())
                 } else {
                     String::new()
                 };

Comment 2

[Valentin Gosu [:valentin] (he/him)]

I'm OK with landing this fix today - nice improvement - but ultimately I'd like to avoid serializing into mFlatHttpRequestHeaders and instead just put Headers into the vec in Http3Stream::GetHeadersString .

Comment 3

[Max Inden]

Attachment: Rust unit tests for parse_headers

I took another look at the above proposal. I would only feel comfortable proposing it, if we had unit tests for `parse_headers`. That said, `./mach rusttests` does not compile nsstring and thus can not run unit tests written in `neqo_glue`.

For the future, I attached a diff for unit tests and the compile time error.

```
 0:06.45 error: linking with `/home/mxinden/code/github.com/mozilla-firefox/firefox/build/cargo-linker` failed: exit status: 1
 0:06.45   |
 0:06.47   = note:  "/home/mxinden/code/github.com/mozilla-firefox/firefox/build/cargo-linker" "-m64" "/tmp/rustcYI2ZgU/symbols.o" "<100 object files omitted>" "-Wl,--as-needed" "-Wl,-Bstatic" "/home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/x86_64-unknown-linux-gnu/debug/deps/{libneqo_udp-edb84b22deb68ef8.rlib,libquinn_udp-d18d5a87d2fffa01.rlib,libsocket2-858d7c139e8ca3b2.rlib}.rlib" "<sysroot>/lib/rustlib/x86_64-unknown-linux-gnu/lib/{libtest-*,libgetopts-*,libunicode_width-*,librustc_std_workspace_std-*}.rlib" "/home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/x86_64-unknown-linux-gnu/debug/deps/{libstatic_prefs-c5a0ca0905e284cd.rlib,libxpcom-df9ccc44ab713669.rlib,libthreadbound-812e297347d551fd.rlib,libthin_vec-684f7719ff41b00c.rlib,libnserror-4cdbb4275af645ba.rlib,libnsstring-f9632f1e6f735f2b.rlib,libencoding_rs-ddf6b83e96862525.rlib,libneqo_http3-bb7059181a4ede82.rlib,libsfv-8338f8258b52f4d7.rlib,librust_decimal-e0d9a39db0caf1a5.rlib,libarrayvec-661c91aadea2064f.rlib,libdata_encoding-2b5fb798caf0fccd.rlib,libneqo_qpack-46619560b1d80c88.rlib,libneqo_transport-4e08c2cecfd0f196.rlib,libmtu-1ddb205ab666510c.rlib,libenumset-aeb302ce840c81b3.rlib,libneqo_crypto-0c8cebf92d036e1d.rlib,libneqo_common-6d479a12073b5057.rlib,libstrum-16a417da3a202b95.rlib,libstrum-93309871cee63da5.rlib,libenum_map-f89ac945a961e727.rlib,libqlog-f9f75ecf81cb22cc.rlib,libserde_with-4c5a63cd64538ab6.rlib,libenv_logger-9c184498eddcf1a2.rlib,libtermcolor-0ca91d71055f70bf.rlib,libfirefox_on_glean-618f0183eee8e181.rlib,libmozbuild-11d78eaba4117db1.rlib,libglean-9eef2b36ac265dda.rlib,libwhatsys-0802391e0582dc9d.rlib,libglean_core-c9f90ac42f999756.rlib,libzeitstempel-f65a203199028ddb.rlib,libtime-64b4c896ef91a017.rlib,libuniffi-1409b3ec44379ced.rlib,libuniffi_core-cfbb28ed76d3249f.rlib,libstatic_assertions-c94044ce54b10438.rlib,libbytes-844077e6cd296300.rlib,libanyhow-e71ff2a59a393470.rlib,libflate2-681ce7e0b634a89d.rlib,libminiz_oxide-946f2067ef74193d.rlib,libadler-d42badeb31393f37.rlib,libcrc32fast-db303833a7afec14.rlib,libserde_json-d65c18fdcfec6a66.rlib,libmemchr-a998e5af079ae889.rlib,libryu-33d53fd947b730cd.rlib,libitoa-d313831906b3a4bc.rlib,libindexmap-dc142375124052f9.rlib,libhashbrown-7765bb431a1ebe43.rlib,libfoldhash-de519437d1a3858b.rlib,libequivalent-c1ecbcb3eb31b820.rlib,liballocator_api2-b244ddeaa3ace8e5.rlib,librkv-6762008c5f94e9a6.rlib,libordered_float-4ac6bfc62d65a66e.rlib,libarrayref-9b35edd2debd0bed.rlib,libbitflags-7213b44a04a27424.rlib,libbincode-d010d66270a89c48.rlib,libid_arena-3beb862ee2aafa81.rlib,liblazy_static-f6f61bd179ca8e11.rlib,liburl-a6a878ce0993f618.rlib,libidna-3d5cf4620516dca7.rlib,libidna_adapter-160f8b5955b10ae6.rlib,libicu_normalizer-610539daf71f620c.rlib,libicu_normalizer_data-9aa9a3af775bfa43.rlib,libwrite16-c7020afb95b507ec.rlib,libutf8_iter-178901d261e12be1.rlib,libutf16_iter-70759ef0a04a5e0c.rlib,libicu_properties-19ce4fad1317714a.rlib,libicu_properties_data-df869c6511355834.rlib,libicu_locid_transform-395ede5f2f5a858c.rlib,libicu_locid_transform_data-da65edaac28e1f77.rlib,libicu_collections-947be97388edf049.rlib,libicu_provider-0114b73470a94147.rlib,libicu_locid-8996c9b3f75716fb.rlib,liblitemap-a878b749c25b638a.rlib,libtinystr-2f81c7908bbc2a72.rlib,libzerovec-f23373a14ce44fe2.rlib,libwriteable-208de7c77cbb972d.rlib,libyoke-2d6ade5ba05ad57c.rlib,libzerofrom-f82231788c2f74d2.rlib,libstable_deref_trait-66231aea468983f6.rlib,libunicode_bidi-4d06cbb7cd6b5578.rlib,libsmallvec-d7f11ff4544bb0e6.rlib,libform_urlencoded-b4b96f7dfe9503e0.rlib,libpercent_encoding-96a3e1acdfd4a20f.rlib,libthiserror-e975ae74529e45e9.rlib,libthiserror-8b13a012da9afe07.rlib,libwr_malloc_size_of-ec3cb7d89928d6b6.rlib,libeuclid-00c964295d4a15dd.rlib,libapp_units-6098d14f359084e9.rlib,liblog-7248124f109b2dd4.rlib,libcrossbeam_channel-03b2325ce01b27f2.rlib,libcrossbeam_utils-1b6a15d4e8f7d1fc.rlib,libuuid-66da514a61c66aef.rlib,libgetrandom-a0845902b8f16a33.rlib,libgetrandom-4e333ac284dc0f0f.rlib,liblibc-47cdf1a0198cf2ca.rlib,libcfg_if-1d3126ddc18e3538.rlib,libonce_cell-f48b0a2ce9a3371e.rlib,libchrono-be9ad8e38ac4cedf.rlib,libnum_traits-8f3db85c8aecd427.rlib,libiana_time_zone-cceec6fe7f025f5e.rlib,libserde-2feb0bfcad6fe61a.rlib}.rlib" "<sysroot>/lib/rustlib/x86_64-unknown-linux-gnu/lib/{libstd-*,libpanic_unwind-*,libobject-*,libmemchr-*,libaddr2line-*,libgimli-*,librustc_demangle-*,libstd_detect-*,libhashbrown-*,librustc_std_workspace_alloc-*,libminiz_oxide-*,libadler2-*,libunwind-*,libcfg_if-*,liblibc-*,liballoc-*,librustc_std_workspace_core-*,libcore-*,libcompiler_builtins-*}.rlib" "-Wl,-Bdynamic" "-lnssutil3" "-lnss3" "-lssl3" "-lplds4" "-lplc4" "-lnspr4" "-lgcc_s" "-lutil" "-lrt" "-lpthread" "-lm" "-ldl" "-lc" "-Wl,--eh-frame-hdr" "-Wl,-z,noexecstack" "-L" "/home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/dist/bin" "-L" "/home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/security/nss/lib/nss/nss_nss3" "-L" "/home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/security/nss/lib/ssl/ssl_ssl3" "-L" "/home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/config/external/nspr/pr" "-L" "<sysroot>/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-o" "/home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/x86_64-unknown-linux-gnu/debug/deps/neqo_glue-50b2f4afb9726e6c" "-Wl,--gc-sections" "-pie" "-Wl,-z,relro,-z,now" "-Wl,-O1" "-nodefaultlibs"
 0:06.47   = note: some arguments are omitted. use `--verbose` to show all linker arguments
 0:06.47   = note: ld.lld: error: undefined symbol: Gecko_FinalizeCString
 0:06.47           >>> referenced by lib.rs:893 (xpcom/rust/nsstring/src/lib.rs:893)
 0:06.47           >>>               nsstring-f9632f1e6f735f2b.2kw6zt5yr1hpysgscc4dd9dvg.rcgu.o:(core::ptr::drop_in_place$LT$nsstring..nsCString$GT$::h046bf2db7ca38e94) in archive /home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/x86_64-unknown-linux-gnu/debug/deps/libnsstring-f9632f1e6f735f2b.rlib
 0:06.47           
 0:06.47           ld.lld: error: undefined symbol: Gecko_GetErrorName
 0:06.47           >>> referenced by lib.rs:39 (xpcom/rust/nserror/src/lib.rs:39)
 0:06.47           >>>               nserror-4cdbb4275af645ba.do9ydz6dmfkj2bguvw8sh8u1q.rcgu.o:(_$LT$nserror..nsresult$u20$as$u20$core..fmt..Debug$GT$::fmt::h0afb2287e7cdd62e) in archive /home/mxinden/code/github.com/mozilla-firefox/firefox/obj-x86_64-pc-linux-gnu/x86_64-unknown-linux-gnu/debug/deps/libnserror-4cdbb4275af645ba.rlib
 0:06.47           clang: error: linker command failed with exit code 1 (use -v to see invocation)
 0:06.47           
 0:06.47 warning: `neqo_glue` (lib test) generated 2 warnings
 0:06.47 error: could not compile `neqo_glue` (lib test) due to 1 previous error; 2 warnings emitted
```

Comment 4

[Max Inden]

Additional information:

Rust tests have one major restriction: they cannot link against Gecko symbols. Therefore, Rust tests cannot be used for crates that use Gecko crates like nsstring and xpcom.

https://firefox-source-docs.mozilla.org/testing-rust-code/#rust-tests

Comment 5

[Max Inden]

Attachment: Bug 1965246 - reduce allocations in neqo_glue::parse_headers r=jesup,valentin

Previously neqo_glue::parse_headers would allocate both header name
and header value twice while parsing. With this commit, both are
allocated only once.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b6d06b763da8
https://hg.mozilla.org/integration/autoland/rev/6bdfe72c7641
reduce allocations in neqo_glue::parse_headers r=valentin,necko-reviewers

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/6bdfe72c7641

Comment 8

[Mayank Bansal]

curious if there are any tests that would show improvements from this fix.

Comment 9

[Max Inden]

I am not aware of a micro benchmark that would be sensitive enough to detect the performance impact of https://phabricator.services.mozilla.com/D253684. Our assumption is, that removing two unnecessary allocations will always have a positive impact. Ideally we would have such a benchmark.