Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use 64 bits for incremental cache in-file positions #104164

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Nov 8, 2022

We currently use a 32-bit integer to encode byte positions into the incremental cache.
This is not enough when the query chache file is >4GB.

As the overflow check was a debug_assert, it was removed in released compilers, making compilation succeed silently.
At the next compilation, cache decoding would try to read unrelated data because of garbled file position, triggering an ICE.

Fixes #79786
(I'm closing that bug since it the original report and the subsequent questions are probably different instances. A new bug should be opened for new instances of that ICE.)

@rustbot
Copy link
Collaborator

rustbot commented Nov 8, 2022

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 8, 2022
@compiler-errors

This comment was marked as outdated.

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 8, 2022

📌 Commit c49e250 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2022
@Xuanwo
Copy link
Contributor

Xuanwo commented Nov 10, 2022

Hi, is it possible to merge this PR a bit more quickly? We (at databend) are affected by this issue every day. Thanks in advance.

@compiler-errors
Copy link
Member

Nominating beta backport, since the change is pretty self-contained.

@compiler-errors compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 10, 2022
@cjgillot
Copy link
Contributor Author

@bors rollup=never
(in case there is a perf effect, and to get it merged a bit faster)

@bors
Copy link
Contributor

bors commented Nov 10, 2022

⌛ Testing commit c49e250 with merge c1a859b...

@bors
Copy link
Contributor

bors commented Nov 10, 2022

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing c1a859b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 10, 2022
@bors bors merged commit c1a859b into rust-lang:master Nov 10, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 10, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c1a859b): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [0.8%, 2.9%] 20
Regressions ❌
(secondary)
1.7% [1.2%, 2.3%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.7% [-8.3%, -3.8%] 3
All ❌✅ (primary) 1.4% [0.8%, 2.9%] 20

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.3% [3.1%, 3.4%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.0% [-2.0%, -2.0%] 1
All ❌✅ (primary) - - 0

@cjgillot cjgillot deleted the u64-cache branch November 14, 2022 21:50
@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 18, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.67.0, 1.66.0 Nov 20, 2022
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 20, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 20, 2022
…mulacrum

[beta] backport

*  Use nominal_obligations_without_const in wf for FnDef rust-lang#104180
*  Don't silently eat label before block in block-like expr rust-lang#103986
*  Revert "Update CI to use Android NDK r25b" rust-lang#104628
*  rustdoc: Resolve doc links in external traits having local impls rust-lang#104364
*  Use 64 bits for incremental cache in-file positions rust-lang#104164
*  rustdoc: Do not add external traits to the crate in register_res rust-lang#103649
*  Revert "Normalize opaques with escaping bound vars" rust-lang#103509
* Bump to released stable compiler
* [beta] Update cargo rust-lang#104486

r? `@Mark-Simulacrum`
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Use 64 bits for incremental cache in-file positions

We currently use a 32-bit integer to encode byte positions into the incremental cache.
This is not enough when the query chache file is >4GB.

As the overflow check was a `debug_assert`, it was removed in released compilers, making compilation succeed silently.
At the next compilation, cache decoding would try to read unrelated data because of garbled file position, triggering an ICE.

Fixes rust-lang#79786
(I'm closing that bug since it the original report and the subsequent questions are probably different instances. A new bug should be opened for new instances of that ICE.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
8 participants