[Kobzol]

Opening for perf. An alternative could be to have a separate interner only for integer literals.

[Kobzol]

@bors try @rust-timer queue

[rust-bors]

💔 Test for 5b7a44b failed: CI. Failed jobs:

• try - dist-aarch64-linux-quick (web logs, enhanced plaintext logs)
• try - dist-x86_64-linux-quick (web logs, enhanced plaintext logs)

[joshtriplett]

@Kobzol Was just looking at this one. If we add an interner for integers, we could have a format in which we store most integers inline and then intern the unusual ones. For instance, store values in the range of an i31 directly, store other values interned and set the high bit to indicate an interned IDs.

(We could also get cleverer with the encoding, using some subset of the tricks ARM literal encoding does, but in practice I expect we'd pay much more in encoding/decoding than we'd save by inlining large-but-low-entropy values.

[Kobzol]

Yeah, that would be my next attempt, to have a 32-bit number that can be either inlined or interned (for the rare large literals). I first wanted to see perf with just storing a string though (have to fix the code first..).

[Kobzol]

@bors try

[rust-log-analyzer]

The job test-pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking idna v1.1.0
error[E0599]: no method named `get` found for struct `IntLiteral` in the current scope
   --> src/tools/clippy/clippy_utils/src/consts.rs:424:47
    |
424 |         LitKind::Int(n, _) => Constant::Int(n.get()),
    |                                               ^^^ method not found in `IntLiteral`

error[E0616]: field `0` of struct `IntLiteral` is private
   --> src/tools/clippy/clippy_utils/src/consts.rs:503:90
    |
503 |         ExprKind::Lit(lit) if let LitKind::Int(val, _) = lit.node => Some(FullInt::U(val.0)),
    |                                                                                          ^ private field

error[E0616]: field `0` of struct `IntLiteral` is private
   --> src/tools/clippy/clippy_utils/src/consts.rs:508:33
    |
508 |             Some(FullInt::S(val.0.cast_signed().wrapping_neg()))
    |                                 ^ private field

[RUSTC-TIMING] cargo_platform test:false 0.124
    Checking cargo_metadata v0.23.1
[RUSTC-TIMING] toml test:false 0.250
   Compiling nom v7.1.3
[RUSTC-TIMING] idna test:false 0.233
    Checking regex-automata v0.4.13
error[E0308]: mismatched types
   --> src/tools/clippy/clippy_utils/src/lib.rs:669:49
    |
668 |         ExprKind::Lit(lit) => match lit.node {
    |                                     -------- this expression has type `rustc_ast::LitKind`
669 |             LitKind::Bool(false) | LitKind::Int(Pu128(0), _) => true,
    |                                                 ^^^^^^^^ expected `IntLiteral`, found `Pu128`

error[E0369]: binary operation `<=` cannot be applied to type `IntLiteral`
    --> src/tools/clippy/clippy_utils/src/lib.rs:678:22
     |
 678 |                 && v <= 32
     |                    - ^^ -- {integer}
     |                    |
     |                    IntLiteral
     |
note: `IntLiteral` does not implement `std::cmp::PartialOrd<{integer}>`
    --> compiler/rustc_ast/src/ast.rs:2341:1
     |
2341 | pub struct IntLiteral(Symbol);
     | ^^^^^^^^^^^^^^^^^^^^^ `IntLiteral` is defined in another crate

error[E0308]: mismatched types
   --> src/tools/clippy/clippy_utils/src/lib.rs:713:33
    |
713 |                     return v == 0 && ty.basic_res().is_diag_item(cx, sym::Vec);
    |                            -    ^ expected `IntLiteral`, found integer
    |                            |
    |                            expected because this is `IntLiteral`

error[E0308]: mismatched types
    --> src/tools/clippy/clippy_utils/src/lib.rs:1356:21

[rust-bors]

☀️ Try build successful (CI)
Build commit: b970737 (b970737982e3cb50952f83c328334a8418a0c71c)
Base parent: 8d1a764 (8d1a76430406c877b35d0b627e7f796dcf0dfeca)

[panstromek]

Related discussion (and experiment): #159008 (comment)

[rust-timer]

Finished benchmarking commit (b970737): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.3%, 0.7%]	9
Regressions ❌ (secondary)	0.5%	[0.2%, 0.7%]	26
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.1%]	6
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	0.2%	[-0.2%, 0.7%]	15

Max RSS (memory usage)

Results (primary -1.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.6%	[1.1%, 2.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.9%	[-6.9%, -6.9%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.2%	[-6.9%, 2.1%]	3

Cycles

Results (primary -2.9%, secondary -2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.9%	[-2.9%, -2.9%]	1
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-2.9%	[-2.9%, -2.9%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.893s -> 491.79s (0.39%)
Artifact size: 408.60 MiB -> 408.69 MiB (0.02%)

[Kobzol]

This is perf. with the integer literal interning approach. Seems that @nnethercote was right, and this isn't worth it!

[joshtriplett]

@Kobzol For the integer literal inlining approach, assuming I've read the code correctly, it's doing an unsigned packing, so (-1) as u128 is going to get interned, right? If so, you might consider a signed packing, inlining the values that an i31 could hold.

[Kobzol]

I don't think that the specific interning implementation matters that much, because we didn't see any max RSS wins, and those won't really change much regardless of how we intern. It seems that reducing LitKind to 8 bytes just doesn't help all that much.

[rust-bors]

☔ The latest upstream changes (presumably #165151) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.