From 487bdeb519a821298b76a35d8c437ef2e733da0d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 22 Jun 2023 09:19:58 +1000 Subject: [PATCH 1/8] Improve ordering and naming of CGUs for non-incremental builds. Currently there are two problems. First, the CGUS don't end up in size order. The merging loop does sort by size on each iteration, but we don't sort after the final merge, so typically there is one CGU out of place. (And sometimes we don't enter the merging loop at all, in which case they end up in random order.) Second, we then assign names that differ only by a numeric suffix, and then we sort them lexicographically by name, giving us an order like this: regex.f10ba03eb5ec7975-cgu.1 regex.f10ba03eb5ec7975-cgu.10 regex.f10ba03eb5ec7975-cgu.11 regex.f10ba03eb5ec7975-cgu.12 regex.f10ba03eb5ec7975-cgu.13 regex.f10ba03eb5ec7975-cgu.14 regex.f10ba03eb5ec7975-cgu.15 regex.f10ba03eb5ec7975-cgu.2 regex.f10ba03eb5ec7975-cgu.3 regex.f10ba03eb5ec7975-cgu.4 regex.f10ba03eb5ec7975-cgu.5 regex.f10ba03eb5ec7975-cgu.6 regex.f10ba03eb5ec7975-cgu.7 regex.f10ba03eb5ec7975-cgu.8 regex.f10ba03eb5ec7975-cgu.9 These two problems are really annoying when debugging and profiling the CGUs. This commit ensures CGUs are sorted by name *and* reverse sorted by size. This involves (a) one extra sort by size operation, and (b) padding the numeric indices with zeroes, e.g. `regex.f10ba03eb5ec7975-cgu.01`. (Note that none of this applies for incremental builds, where a different hash-based CGU naming scheme is used.) --- .../rustc_monomorphize/src/partitioning.rs | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index 97e3748b8b826..10a496cf993d2 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -368,6 +368,7 @@ fn merge_codegen_units<'tcx>( let cgu_name_builder = &mut CodegenUnitNameBuilder::new(cx.tcx); + // Rename the newly merged CGUs. if cx.tcx.sess.opts.incremental.is_some() { // If we are doing incremental compilation, we want CGU names to // reflect the path of the source level module they correspond to. @@ -404,18 +405,38 @@ fn merge_codegen_units<'tcx>( } } } + + // A sorted order here ensures what follows can be deterministic. + codegen_units.sort_by(|a, b| a.name().as_str().cmp(b.name().as_str())); } else { - // If we are compiling non-incrementally we just generate simple CGU - // names containing an index. + // When compiling non-incrementally, we rename the CGUS so they have + // identical names except for the numeric suffix, something like + // `regex.f10ba03eb5ec7975-cgu.N`, where `N` varies. + // + // It is useful for debugging and profiling purposes if the resulting + // CGUs are sorted by name *and* reverse sorted by size. (CGU 0 is the + // biggest, CGU 1 is the second biggest, etc.) + // + // So first we reverse sort by size. Then we generate the names with + // zero-padded suffixes, which means they are automatically sorted by + // names. The numeric suffix width depends on the number of CGUs, which + // is always greater than zero: + // - [1,9] CGUS: `0`, `1`, `2`, ... + // - [10,99] CGUS: `00`, `01`, `02`, ... + // - [100,999] CGUS: `000`, `001`, `002`, ... + // - etc. + // + // If we didn't zero-pad the sorted-by-name order would be `XYZ-cgu.0`, + // `XYZ-cgu.1`, `XYZ-cgu.10`, `XYZ-cgu.11`, ..., `XYZ-cgu.2`, etc. + codegen_units.sort_by_key(|cgu| cmp::Reverse(cgu.size_estimate())); + let num_digits = codegen_units.len().ilog10() as usize + 1; for (index, cgu) in codegen_units.iter_mut().enumerate() { + let suffix = format!("{index:0num_digits$}"); let numbered_codegen_unit_name = - cgu_name_builder.build_cgu_name_no_mangle(LOCAL_CRATE, &["cgu"], Some(index)); + cgu_name_builder.build_cgu_name_no_mangle(LOCAL_CRATE, &["cgu"], Some(suffix)); cgu.set_name(numbered_codegen_unit_name); } } - - // A sorted order here ensures what follows can be deterministic. - codegen_units.sort_by(|a, b| a.name().as_str().cmp(b.name().as_str())); } fn internalize_symbols<'tcx>( From 666b1b68a7200513872303f1bc8136088a09d4ad Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 22 Jun 2023 08:58:48 +1000 Subject: [PATCH 2/8] Tweak thread names for CGU processing. For non-incremental builds on Unix, currently all the thread names look like `opt regex.f10ba03eb5ec7975-cgu.0`. But they are truncated by `pthread_setname` to `opt regex.f10ba`, hiding the numeric suffix that distinguishes them. This is really annoying when using a profiler like Samply. This commit changes these thread names to a form like `opt cgu.0`, which is much better. --- compiler/rustc_codegen_ssa/src/back/write.rs | 63 ++++++++++++------- .../rustc_monomorphize/src/partitioning.rs | 3 + 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index 51ac441a7a425..dbe6f466a2a58 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -698,28 +698,49 @@ impl WorkItem { /// Generate a short description of this work item suitable for use as a thread name. fn short_description(&self) -> String { - // `pthread_setname()` on *nix is limited to 15 characters and longer names are ignored. - // Use very short descriptions in this case to maximize the space available for the module name. - // Windows does not have that limitation so use slightly more descriptive names there. + // `pthread_setname()` on *nix ignores anything beyond the first 15 + // bytes. Use short descriptions to maximize the space available for + // the module name. + #[cfg(not(windows))] + fn desc(short: &str, _long: &str, name: &str) -> String { + // The short label is three bytes, and is followed by a space. That + // leaves 11 bytes for the CGU name. How we obtain those 11 bytes + // depends on the the CGU name form. + // + // - Non-incremental, e.g. `regex.f10ba03eb5ec7975-cgu.0`: the part + // before the `-cgu.0` is the same for every CGU, so use the + // `cgu.0` part. The number suffix will be different for each + // CGU. + // + // - Incremental (normal), e.g. `2i52vvl2hco29us0`: use the whole + // name because each CGU will have a unique ASCII hash, and the + // first 11 bytes will be enough to identify it. + // + // - Incremental (with `-Zhuman-readable-cgu-names`), e.g. + // `regex.f10ba03eb5ec7975-re_builder.volatile`: use the whole + // name. The first 11 bytes won't be enough to uniquely identify + // it, but no obvious substring will, and this is a rarely used + // option so it doesn't matter much. + // + assert_eq!(short.len(), 3); + let name = if let Some(index) = name.find("-cgu.") { + &name[index + 1..] // +1 skips the leading '-'. + } else { + name + }; + format!("{short} {name}") + } + + // Windows has no thread name length limit, so use more descriptive names. + #[cfg(windows)] + fn desc(_short: &str, long: &str, name: &str) -> String { + format!("{long} {name}") + } + match self { - WorkItem::Optimize(m) => { - #[cfg(windows)] - return format!("optimize module {}", m.name); - #[cfg(not(windows))] - return format!("opt {}", m.name); - } - WorkItem::CopyPostLtoArtifacts(m) => { - #[cfg(windows)] - return format!("copy LTO artifacts for {}", m.name); - #[cfg(not(windows))] - return format!("copy {}", m.name); - } - WorkItem::LTO(m) => { - #[cfg(windows)] - return format!("LTO module {}", m.name()); - #[cfg(not(windows))] - return format!("LTO {}", m.name()); - } + WorkItem::Optimize(m) => desc("opt", "optimize module {}", &m.name), + WorkItem::CopyPostLtoArtifacts(m) => desc("cpy", "copy LTO artifacts for {}", &m.name), + WorkItem::LTO(m) => desc("lto", "LTO module {}", m.name()), } } } diff --git a/compiler/rustc_monomorphize/src/partitioning.rs b/compiler/rustc_monomorphize/src/partitioning.rs index 10a496cf993d2..e663f4486f7fe 100644 --- a/compiler/rustc_monomorphize/src/partitioning.rs +++ b/compiler/rustc_monomorphize/src/partitioning.rs @@ -431,6 +431,9 @@ fn merge_codegen_units<'tcx>( codegen_units.sort_by_key(|cgu| cmp::Reverse(cgu.size_estimate())); let num_digits = codegen_units.len().ilog10() as usize + 1; for (index, cgu) in codegen_units.iter_mut().enumerate() { + // Note: `WorkItem::short_description` depends on this name ending + // with `-cgu.` followed by a numeric suffix. Please keep it in + // sync with this code. let suffix = format!("{index:0num_digits$}"); let numbered_codegen_unit_name = cgu_name_builder.build_cgu_name_no_mangle(LOCAL_CRATE, &["cgu"], Some(suffix)); From 35cfac1e2b69c8bfe531c60e64ec9999cbd011ed Mon Sep 17 00:00:00 2001 From: Petr Sumbera Date: Tue, 27 Jun 2023 14:28:44 +0200 Subject: [PATCH 3/8] [PATCH] Fix build on Solaris where fd-lock cannot be used. --- src/bootstrap/bin/main.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/bootstrap/bin/main.rs b/src/bootstrap/bin/main.rs index 7ce4599c4241f..30dfa81c6f7fb 100644 --- a/src/bootstrap/bin/main.rs +++ b/src/bootstrap/bin/main.rs @@ -5,9 +5,11 @@ //! parent directory, and otherwise documentation can be found throughout the `build` //! directory in each respective module. -use std::fs::OpenOptions; +#[cfg(all(any(unix, windows), not(target_os = "solaris")))] use std::io::Write; -use std::{env, fs, process}; +#[cfg(all(any(unix, windows), not(target_os = "solaris")))] +use std::process; +use std::{env, fs}; #[cfg(all(any(unix, windows), not(target_os = "solaris")))] use bootstrap::t; @@ -32,7 +34,7 @@ fn main() { }; build_lock = - fd_lock::RwLock::new(t!(OpenOptions::new().write(true).create(true).open(&path))); + fd_lock::RwLock::new(t!(fs::OpenOptions::new().write(true).create(true).open(&path))); _build_lock_guard = match build_lock.try_write() { Ok(mut lock) => { t!(lock.write(&process::id().to_string().as_ref())); @@ -85,7 +87,7 @@ fn main() { // HACK: Since the commit script uses hard links, we can't actually tell if it was installed by x.py setup or not. // We could see if it's identical to src/etc/pre-push.sh, but pre-push may have been modified in the meantime. // Instead, look for this comment, which is almost certainly not in any custom hook. - if std::fs::read_to_string(pre_commit).map_or(false, |contents| { + if fs::read_to_string(pre_commit).map_or(false, |contents| { contents.contains("https://github.com/rust-lang/rust/issues/77620#issuecomment-705144570") }) { println!( From 0c10eb0b6af6aee3d88c58cd30cfe47ae85896eb Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 27 Jun 2023 15:05:43 +0200 Subject: [PATCH 4/8] Fix display of long items in search results --- src/librustdoc/html/static/css/rustdoc.css | 30 +++++++++++++++++----- src/librustdoc/html/static/js/search.js | 8 +++--- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/librustdoc/html/static/css/rustdoc.css b/src/librustdoc/html/static/css/rustdoc.css index b6d90091bbaef..ccfd4cc5b875b 100644 --- a/src/librustdoc/html/static/css/rustdoc.css +++ b/src/librustdoc/html/static/css/rustdoc.css @@ -8,6 +8,7 @@ :root { --nav-sub-mobile-padding: 8px; + --search-typename-width: 6.75rem; } /* See FiraSans-LICENSE.txt for the Fira Sans license. */ @@ -869,14 +870,11 @@ so that we can apply CSS-filters to change the arrow color in themes */ gap: 1em; } -.search-results > a > div { - flex: 1; -} - .search-results > a > div.desc { white-space: nowrap; text-overflow: ellipsis; overflow: hidden; + flex: 2; } .search-results a:hover, @@ -884,6 +882,12 @@ so that we can apply CSS-filters to change the arrow color in themes */ background-color: var(--search-result-link-focus-background-color); } +.search-results .result-name { + display: flex; + align-items: center; + justify-content: start; + flex: 3; +} .search-results .result-name span.alias { color: var(--search-results-alias-color); } @@ -891,10 +895,14 @@ so that we can apply CSS-filters to change the arrow color in themes */ color: var(--search-results-grey-color); } .search-results .result-name .typename { - display: inline-block; color: var(--search-results-grey-color); font-size: 0.875rem; - width: 6.25rem; + width: var(--search-typename-width); +} +.search-results .result-name .path { + word-break: break-all; + max-width: calc(100% - var(--search-typename-width)); + display: inline-block; } .popover { @@ -1730,6 +1738,16 @@ in source-script.js .search-results > a > div.desc, .item-table > li > div.desc { padding-left: 2em; } + .search-results .result-name { + display: block; + } + .search-results .result-name .typename { + width: initial; + margin-right: 0; + } + .search-results .result-name .typename, .search-results .result-name .path { + display: inline; + } .source-sidebar-expanded .source .sidebar { max-width: 100vw; diff --git a/src/librustdoc/html/static/js/search.js b/src/librustdoc/html/static/js/search.js index b8cc0a6db714d..345750395f06c 100644 --- a/src/librustdoc/html/static/js/search.js +++ b/src/librustdoc/html/static/js/search.js @@ -2024,9 +2024,11 @@ function initSearch(rawSearchIndex) { resultName.insertAdjacentHTML( "beforeend", - `${typeName}` - + ` ${item.displayPath}${name}` - ); + `\ +${typeName}\ +
\ + ${item.displayPath}${name}\ +
`); link.appendChild(resultName); const description = document.createElement("div"); From a716c79c29a8fe5eb3b7e04e736c596d9fbd29dc Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 27 Jun 2023 16:26:22 +0200 Subject: [PATCH 5/8] Update GUI tests --- tests/rustdoc-gui/search-result-color.goml | 16 +++++----- tests/rustdoc-gui/search-result-display.goml | 32 ++++++++++++++++++-- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/tests/rustdoc-gui/search-result-color.goml b/tests/rustdoc-gui/search-result-color.goml index e3c628b366fbd..7a7785fd9ac30 100644 --- a/tests/rustdoc-gui/search-result-color.goml +++ b/tests/rustdoc-gui/search-result-color.goml @@ -61,7 +61,7 @@ assert-css: ( {"color": "#c5c5c5"}, ) assert-css: ( - "//*[@class='result-name']/*[text()='test_docs::']", + "//*[@class='result-name']//*[text()='test_docs::']", {"color": "#0096cf"}, ) @@ -138,7 +138,7 @@ call-function: ( move-cursor-to: ".search-input" focus: ".search-input" // To ensure the `` container isnt focus or hover. assert-css: ( - "//*[@class='result-name']/*[text()='test_docs::']/ancestor::a", + "//*[@class='result-name']//*[text()='test_docs::']/ancestor::a", {"color": "#0096cf", "background-color": "transparent"}, ALL, ) @@ -146,11 +146,11 @@ assert-css: ( // Checking color and background on hover. move-cursor-to: "//*[@class='desc'][text()='Just a normal struct.']" assert-css: ( - "//*[@class='result-name']/*[text()='test_docs::']", + "//*[@class='result-name']//*[text()='test_docs::']", {"color": "#fff"}, ) assert-css: ( - "//*[@class='result-name']/*[text()='test_docs::']/ancestor::a", + "//*[@class='result-name']//*[text()='test_docs::']/ancestor::a", {"color": "#fff", "background-color": "rgb(60, 60, 60)"}, ) @@ -173,7 +173,7 @@ assert-css: ( {"color": "#ddd"}, ) assert-css: ( - "//*[@class='result-name']/*[text()='test_docs::']", + "//*[@class='result-name']//*[text()='test_docs::']", {"color": "#ddd"}, ) @@ -250,7 +250,7 @@ call-function: ( move-cursor-to: ".search-input" focus: ".search-input" // To ensure the `` container isnt focus or hover. assert-css: ( - "//*[@class='result-name']/*[text()='test_docs::']/ancestor::a", + "//*[@class='result-name']//*[text()='test_docs::']/ancestor::a", {"color": "#ddd", "background-color": "transparent"}, ) @@ -270,7 +270,7 @@ assert-css: ( {"color": "#000"}, ) assert-css: ( - "//*[@class='result-name']/*[text()='test_docs::']", + "//*[@class='result-name']//*[text()='test_docs::']", {"color": "#000"}, ) @@ -347,7 +347,7 @@ call-function: ( move-cursor-to: ".search-input" focus: ".search-input" // To ensure the `` container isnt focus or hover. assert-css: ( - "//*[@class='result-name']/*[text()='test_docs::']/ancestor::a", + "//*[@class='result-name']//*[text()='test_docs::']/ancestor::a", {"color": "#000", "background-color": "transparent"}, ) diff --git a/tests/rustdoc-gui/search-result-display.goml b/tests/rustdoc-gui/search-result-display.goml index 6593c1a9c45a5..f4c0e3eb04731 100644 --- a/tests/rustdoc-gui/search-result-display.goml +++ b/tests/rustdoc-gui/search-result-display.goml @@ -1,3 +1,4 @@ +// ignore-tidy-linelength // Checks that the search results have the expected width. go-to: "file://" + |DOC_PATH| + "/test_docs/index.html" set-window-size: (900, 1000) @@ -7,15 +8,40 @@ press-key: 'Enter' wait-for: "#crate-search" // The width is returned by "getComputedStyle" which returns the exact number instead of the // CSS rule which is "50%"... -assert-size: (".search-results div.desc", {"width": 310}) +assert-size: (".search-results div.desc", {"width": 248}) +store-size: (".search-results .result-name .typename", {"width": width}) set-window-size: (600, 100) // As counter-intuitive as it may seem, in this width, the width is "100%", which is why // when computed it's larger. assert-size: (".search-results div.desc", {"width": 566}) // The result set is all on one line. -assert-css: (".search-results .result-name > span:not(.typename)", {"display": "inline"}) -assert-css: (".search-results .result-name > span.typename", {"display": "inline-block"}) +compare-elements-position-near: ( + ".search-results .result-name .typename", + ".search-results .result-name .path", + {"y": 2}, +) +compare-elements-position-near-false: ( + ".search-results .result-name .typename", + ".search-results .result-name .path", + {"x": 5}, +) +// The width of the "typename" isn't fixed anymore in this display mode. +store-size: (".search-results .result-name .typename", {"width": new_width}) +assert: |new_width| < |width| - 10 + +// Check that if the search is too long on mobile, it'll go under the "typename". +go-to: "file://" + |DOC_PATH| + "/test_docs/index.html?search=SuperIncrediblyLongLongLongLongLongLongLongGigaGigaGigaMegaLongLongLongStructName" +wait-for: "#crate-search" +compare-elements-position-near: ( + ".search-results .result-name .typename", + ".search-results .result-name .path", + {"y": 2, "x": 0}, +) +store-size: (".search-results .result-name", {"width": width, "height": height}) +store-size: (".search-results .result-name .path", {"width": sub_width, "height": sub_height}) +assert: |width| < |sub_width| + 8 && |width| > |sub_width| - 8 +assert: |height| < |sub_height| + 8 && |height| > |sub_height| - 8 // Check that the crate filter `