Rollup of 5 pull requests #72228

RalfJung · 2020-05-15T09:12:57Z

Successful merges:

Rework the std::iter::Step trait #69659 (Rework the std::iter::Step trait)
Be less aggressive with DroplessArena/TypedArena growth. #71872 (Be less aggressive with DroplessArena/TypedArena growth.)
Literal error reporting cleanup #72047 (Literal error reporting cleanup)
rustc_driver: factor out computing the exit code #72090 (rustc_driver: factor out computing the exit code)
cmdline: Make target features individually overridable #72094 (cmdline: Make target features individually overridable)

Failed merges:

Co-Authored-By: Nadrieril Feneanar <nadrieril@users.noreply.github.com>

@matklad

The phrasing is from the commit description of 395ee0b by @matklad.

For the given code paths, the amount of space used in the previous chunk is irrelevant. (This will almost never make a difference to behaviour, but it makes the code clearer.)

`DroplessArena` and `TypedArena` use an aggressive growth strategy: the first chunk is 4 KiB, the second is 8 KiB, and it keeps on doubling indefinitely. DHAT profiles show that sometimes this results in large chunks (e.g. 16-128 MiB) that are barely filled. Although these don't contribute to RSS, they clog up the DHAT profiles. This commit changes things so that the doubling stops at 2 MiB. This is large enough that chunk allocations are still rare (you might get 100s instead of 10s of them) but avoids lots of unused space in the worst case. It gives a slight speed-up to cycle counts in some cases.

…ods into one method `unescape_literal` with a mode argument.

…o one method `validate_literal_escape` with a mode argument. This enables simplifying the `match` in `cook_lexer_literal()` and it eliminates 90 lines of repetition :)

…nd tests.

The previous definition did not optimize down to a single add operation, but this version does appear to.

Rework the std::iter::Step trait Previous attempts: rust-lang#43127 rust-lang#62886 rust-lang#68807 Tracking issue: rust-lang#42168 This PR reworks the `Step` trait to be phrased in terms of the *successor* and *predecessor* operations. With this, `Step` hopefully has a consistent identity that can have a path towards stabilization. The proposed trait: ```rust /// Objects that have a notion of *successor* and *predecessor* operations. /// /// The *successor* operation moves towards values that compare greater. /// The *predecessor* operation moves towards values that compare lesser. /// /// # Safety /// /// This trait is `unsafe` because its implementation must be correct for /// the safety of `unsafe trait TrustedLen` implementations, and the results /// of using this trait can otherwise be trusted by `unsafe` code to be correct /// and fulful the listed obligations. pub unsafe trait Step: Clone + PartialOrd + Sized { /// Returns the number of *successor* steps required to get from `start` to `end`. /// /// Returns `None` if the number of steps would overflow `usize` /// (or is infinite, or if `end` would never be reached). /// /// # Invariants /// /// For any `a`, `b`, and `n`: /// /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::forward(&a, n) == Some(b)` /// * `steps_between(&a, &b) == Some(n)` if and only if `Step::backward(&a, n) == Some(a)` /// * `steps_between(&a, &b) == Some(n)` only if `a <= b` /// * Corollary: `steps_between(&a, &b) == Some(0)` if and only if `a == b` /// * Note that `a <= b` does _not_ imply `steps_between(&a, &b) != None`; /// this is the case wheen it would require more than `usize::MAX` steps to get to `b` /// * `steps_between(&a, &b) == None` if `a > b` fn steps_between(start: &Self, end: &Self) -> Option<usize>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, m).and_then(|x| Step::forward_checked(x, n))` /// /// For any `a`, `n`, and `m` where `n + m` does not overflow: /// /// * `Step::forward_checked(a, n).and_then(|x| Step::forward_checked(x, m)) == Step::forward_checked(a, n + m)` /// /// For any `a` and `n`: /// /// * `Step::forward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::forward_checked(&x, 1))` /// * Corollary: `Step::forward_checked(&a, 0) == Some(a)` fn forward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::forward(Step::forward(a, n), m) == Step::forward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_checked(a, n) == Some(Step::forward(a, n))` /// * `Step::forward(a, n) == (0..n).fold(a, |x, _| Step::forward(x, 1))` /// * Corollary: `Step::forward(a, 0) == a` /// * `Step::forward(a, n) >= a` /// * `Step::backward(Step::forward(a, n), n) == a` fn forward(start: Self, count: usize) -> Self { Step::forward_checked(start, count).expect("overflow in `Step::forward`") } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `forward` or `forward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b > a`, it is safe to call `Step::forward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&a, &b) == Some(n)`, /// it is safe to call `Step::forward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::forward_unchecked(a, n)` is equivalent to `Step::forward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn forward_unchecked(start: Self, count: usize) -> Self { Step::forward(start, count) } /// Returns the value that would be obtained by taking the *successor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, returns `None`. /// /// # Invariants /// /// For any `a`, `n`, and `m`: /// /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == n.checked_add(m).and_then(|x| Step::backward_checked(a, x))` /// * `Step::backward_checked(a, n).and_then(|x| Step::backward_checked(x, m)) == try { Step::backward_checked(a, n.checked_add(m)?) }` /// /// For any `a` and `n`: /// /// * `Step::backward_checked(a, n) == (0..n).try_fold(a, |x, _| Step::backward_checked(&x, 1))` /// * Corollary: `Step::backward_checked(&a, 0) == Some(a)` fn backward_checked(start: Self, count: usize) -> Option<Self>; /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// If this would overflow the range of values supported by `Self`, /// this function is allowed to panic, wrap, or saturate. /// The suggested behavior is to panic when debug assertions are enabled, /// and to wrap or saturate otherwise. /// /// Unsafe code should not rely on the correctness of behavior after overflow. /// /// # Invariants /// /// For any `a`, `n`, and `m`, where no overflow occurs: /// /// * `Step::backward(Step::backward(a, n), m) == Step::backward(a, n + m)` /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_checked(a, n) == Some(Step::backward(a, n))` /// * `Step::backward(a, n) == (0..n).fold(a, |x, _| Step::backward(x, 1))` /// * Corollary: `Step::backward(a, 0) == a` /// * `Step::backward(a, n) <= a` /// * `Step::forward(Step::backward(a, n), n) == a` fn backward(start: Self, count: usize) -> Self { Step::backward_checked(start, count).expect("overflow in `Step::backward`") } /// Returns the value that would be obtained by taking the *predecessor* /// of `self` `count` times. /// /// # Safety /// /// It is undefined behavior for this operation to overflow the /// range of values supported by `Self`. If you cannot guarantee that this /// will not overflow, use `backward` or `backward_checked` instead. /// /// # Invariants /// /// For any `a`: /// /// * if there exists `b` such that `b < a`, it is safe to call `Step::backward_unchecked(a, 1)` /// * if there exists `b`, `n` such that `steps_between(&b, &a) == Some(n)`, /// it is safe to call `Step::backward_unchecked(a, m)` for any `m <= n`. /// /// For any `a` and `n`, where no overflow occurs: /// /// * `Step::backward_unchecked(a, n)` is equivalent to `Step::backward(a, n)` #[unstable(feature = "unchecked_math", reason = "niche optimization path", issue = "none")] unsafe fn backward_unchecked(start: Self, count: usize) -> Self { Step::backward(start, count) } } ``` Note that all of these are associated functions and not callable via method syntax; the calling syntax is always `Step::forward(start, n)`. This version of the trait additionally changes the stepping functions to talk their arguments by value. As opposed to previous attempts which provided a "step by one" method directly, this version of the trait only exposes "step by n". There are a few reasons for this: - `Range*`, the primary consumer of `Step`, assumes that the "step by n" operation is cheap. If a single step function is provided, it will be a lot more enticing to implement "step by n" as n repeated calls to "step by one". While this is not strictly incorrect, this behavior would be surprising for anyone used to using `Range<{primitive integer}>`. - With a trivial default impl, this can be easily added backwards-compatibly later. - The debug-wrapping "step by n" needs to exist for `RangeFrom` to be consistent between "step by n" and "step by one" operation. (Note: the behavior is not changed by this PR, but making the behavior consistent is made tenable by this PR.) Three "kinds" of step are provided: `_checked`, which returns an `Option` indicating attempted overflow; (unsuffixed), which provides "safe overflow" behavior (is allowed to panic, wrap, or saturate, depending on what is most convenient for a given type); and `_unchecked`, which is a version which assumes overflow does not happen. Review is appreciated to check that: - The invariants as described on the `Step` functions are enough to specify the "common sense" consistency for successor/predecessor. - Implementation of `Step` functions is correct in the face of overflow and the edges of representable integers. - Added tests of `Step` functions are asserting the correct behavior (and not just the implemented behavior).

…rowth, r=oli-obk Be less aggressive with `DroplessArena`/`TypedArena` growth. `DroplessArena` and `TypedArena` use an aggressive growth strategy: the first chunk is 4 KiB, the second is 8 KiB, and it keeps on doubling indefinitely. DHAT profiles show that sometimes this results in large chunks (e.g. 16-128 MiB) that are barely filled. This commit changes things so that the doubling stops at 2 MiB. This is large enough that chunk allocations are still rare (you might get 100s instead of 10s of them) but avoids lots of unused space in the worst case. It makes the same change to `TypedArena`, too.

…reporting_cleanup, r=petrochenkov Literal error reporting cleanup While doing some performance work, I noticed some code duplication in `librustc_parser/lexer/mod.rs`, so I cleaned it up. This PR is probably best reviewed commit by commit. I'm not sure what the API stability practices for `librustc_lexer` are. Four public methods in `unescape.rs` can be removed, but two are used by clippy, so I left them in for now. I could open a PR for Rust-Analyzer when this one lands. But how do I open a PR for clippy? (Git submodules are frustrating to work with)

@oli-obk

…oli-obk rustc_driver: factor out computing the exit code In a recent Miri PR I [added a convenience wrapper](https://github.com/rust-lang/miri/pull/1405/files#diff-c3d602c5c8035a16699ce9c015bfeceaR125) around `catch_fatal_errors` and `run_compiler` that @oli-obk suggested I could upstream. However, after seeing what could be shared between `rustc_driver::main`, clippy and Miri, really the only thing I found is computing the exit code -- so that's what this PR does. What prevents using the Miri convenience function in `rustc_driver::main` and clippy is that they do extra work inside `catch_fatal_errors`, and while I could abstract that away, clippy actually *computes the callbacks* inside there, and I fond no good way to abstract that and thus gave up. Maybe the clippy thing could be moved out, I am not sure if it ever can actually raise a `FatalErrorMarker` -- someone more knowledgeable in clippy would have to do that.

cmdline: Make target features individually overridable Fixes rust-lang#56527 Previously `-C target-feature=+avx2 -C target-feature=+fma` was equivalent to `-C target-feature=+fma` because the later `-C target-feature` option fully overridden previous `-C target-feature`. With this PR `-C target-feature=+avx2 -C target-feature=+fma` is equivalent to `-C target-feature=+avx2,+fma` and the options are combined. I'm not sure where the comma-separated features in a single option came from (clang uses a scheme with single feature per-option), but logically these features are entirely independent options. So they should be overridable individually as well to be more useful in hierarchical build system, and more consistent with other rustc options and clang behavior as well. Target feature options have a few other issues (rust-lang#44815), but fixing those is going to be a bit more invasive.

RalfJung · 2020-05-15T09:13:19Z

@rustbot modify labels: +rollup
@bors r+ rollup=never p=5

bors · 2020-05-15T09:13:20Z

📌 Commit 7dc2c01 has been approved by RalfJung

bors · 2020-05-15T09:13:47Z

⌛ Testing commit 7dc2c01 with merge 7513582078ed3e22fcf9996e38ccb266a380d55b...

bors · 2020-05-15T11:24:43Z

💔 Test failed - checks-azure

RalfJung · 2020-05-15T11:44:15Z

Same failure as #72226 (comment), so it's probably one of the PRs included in both: #69659, #71872, #72090. The latter is unlikely.

RalfJung · 2020-05-15T11:46:06Z

My guess is #69659.

CAD97 and others added 20 commits April 8, 2020 02:11

Add inherent impls for unchecked math intrinsics

b70e7fd

Redesign the Step trait

2fcfd23

Adjust Step::forward_checked docs for large types

f34322d

Co-Authored-By: Nadrieril Feneanar <nadrieril@users.noreply.github.com>

Small doc improvements.

e734e31

The phrasing is from the commit description of 395ee0b by @matklad.

rustc_driver: factor out computing the exit code

23d880b

rustc_driver::main: more informative return type

51e466d

cmdline: Make target features individually overridable

00dcb66

Fix the new capacity measurement in arenas.

9111d8b

For the given code paths, the amount of space used in the previous chunk is irrelevant. (This will almost never make a difference to behaviour, but it makes the code clearer.)

Unified unescape_{char,byte,str,byte_str,raw_str,raw_byte_str} meth…

1be5d1e

…ods into one method `unescape_literal` with a mode argument.

Unified validate_{byte,str,raw_str,raw_byte_str}_escape methods int…

18cc63d

…o one method `validate_literal_escape` with a mode argument. This enables simplifying the `match` in `cook_lexer_literal()` and it eliminates 90 lines of repetition :)

Replace some usages of the old unescape_ functions in AST, clippy a…

43ae785

…nd tests.

Improve comments in iter::Step

cef616b

Improve Step::forward/backward for optimization

90b1961

The previous definition did not optimize down to a single add operation, but this version does appear to.

improve step_integer_impls macro

d53068e

rustbot added the rollup A PR which is a rollup label May 15, 2020

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 15, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 15, 2020

RalfJung mentioned this pull request May 15, 2020

Rework the std::iter::Step trait #69659

Merged

RalfJung closed this May 15, 2020

RalfJung deleted the rollup-686s7c9 branch May 15, 2020 11:53

This was referenced May 15, 2020

Be less aggressive with DroplessArena/TypedArena growth. #71872

Merged

Rollup of 6 pull requests #72229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollup of 5 pull requests #72228

Rollup of 5 pull requests #72228

RalfJung commented May 15, 2020

RalfJung commented May 15, 2020

bors commented May 15, 2020

bors commented May 15, 2020

bors commented May 15, 2020

RalfJung commented May 15, 2020

RalfJung commented May 15, 2020

Rollup of 5 pull requests #72228

Rollup of 5 pull requests #72228

Conversation

RalfJung commented May 15, 2020

RalfJung commented May 15, 2020

bors commented May 15, 2020

bors commented May 15, 2020

bors commented May 15, 2020

RalfJung commented May 15, 2020

RalfJung commented May 15, 2020