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

Return a finite number of AllocIds per ConstAllocation in Miri #118336

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Nov 26, 2023

Before this, every evaluation of a const slice would produce a new AllocId. So in Miri, this program used to have unbounded memory use:

fn main() {
    loop {
        helper();
    }
}

fn helper() {
    "ouch";
}

Every trip around the loop creates a new AllocId which we need to keep track of a base address for. And the provenance GC can never clean up that AllocId -> u64 mapping, because the AllocId is for a const allocation which will never be deallocated.

So this PR moves the logic of producing an AllocId for a ConstAllocation to the Machine trait, and the implementation that Miri provides will only produce 16 AllocIds for each allocation. The cache is also keyed on the Instance that the const is evaluated in, so that equal consts evaluated in two functions will have disjoint base addresses.

r? RalfJung

@rustbot rustbot added 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 26, 2023
@saethlin
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 26, 2023
@bors
Copy link
Contributor

bors commented Nov 26, 2023

⌛ Trying commit 29b7077 with merge 0182293...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 26, 2023
Add a cache for const_val_to_op

r? `@ghost`
@bors
Copy link
Contributor

bors commented Nov 26, 2023

☀️ Try build successful - checks-actions
Build commit: 0182293 (01822939f6daaf322c811fab3c3f91d1ea582b3f)

@rust-timer

This comment has been minimized.

@rust-timer

This comment was marked as outdated.

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 27, 2023
@saethlin
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 27, 2023
@bors
Copy link
Contributor

bors commented Nov 27, 2023

⌛ Trying commit c57773d with merge 8422d83...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 27, 2023
Add a cache for const_val_to_op

r? `@ghost`
@bors
Copy link
Contributor

bors commented Nov 27, 2023

☀️ Try build successful - checks-actions
Build commit: 8422d83 (8422d83d697036d5904e9bb5495458979a8e5322)

@rust-timer

This comment has been minimized.

@rust-timer

This comment was marked as off-topic.

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Nov 28, 2023
@saethlin saethlin changed the title Add a cache for const_val_to_op Return a finite number of AllocIds per ConstAllocation in Miri Nov 30, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 30, 2023

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@saethlin saethlin marked this pull request as ready for review November 30, 2023 17:36
@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 Jan 23, 2024
@bors
Copy link
Contributor

bors commented Jan 23, 2024

⌛ Testing commit c8a675d with merge a3f746e...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2024
Return a finite number of AllocIds per ConstAllocation in Miri

Before this, every evaluation of a const slice would produce a new AllocId. So in Miri, this program used to have unbounded memory use:
```rust
fn main() {
    loop {
        helper();
    }
}

fn helper() {
    "ouch";
}
```
Every trip around the loop creates a new AllocId which we need to keep track of a base address for. And the provenance GC can never clean up that AllocId -> u64 mapping, because the AllocId is for a const allocation which will never be deallocated.

So this PR moves the logic of producing an AllocId for a ConstAllocation to the Machine trait, and the implementation that Miri provides will only produce 16 AllocIds for each allocation. The cache is also keyed on the Instance that the const is evaluated in, so that equal consts evaluated in two functions will have disjoint base addresses.

r? RalfJung
@bors
Copy link
Contributor

bors commented Jan 23, 2024

💔 Test failed - checks-actions

@bors 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 Jan 23, 2024
@fmease
Copy link
Member

fmease commented Jan 23, 2024

Error: Internal server error occurred while resolving "actions/checkout@v4". Internal server error occurred while resolving "actions/upload-artifact@v3"

@bors retry

@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 Jan 23, 2024
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Secret source: Actions
Prepare workflow directory
Prepare all required actions
Getting action download info
##[error]Internal server error occurred while resolving "actions/checkout@v4". Internal server error occurred while resolving "actions/upload-artifact@v3"

@saethlin
Copy link
Member Author

Same error, but it looks like the queued rollup might have gotten past the flaky spot now.
@bors retry

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2024
Return a finite number of AllocIds per ConstAllocation in Miri

Before this, every evaluation of a const slice would produce a new AllocId. So in Miri, this program used to have unbounded memory use:
```rust
fn main() {
    loop {
        helper();
    }
}

fn helper() {
    "ouch";
}
```
Every trip around the loop creates a new AllocId which we need to keep track of a base address for. And the provenance GC can never clean up that AllocId -> u64 mapping, because the AllocId is for a const allocation which will never be deallocated.

So this PR moves the logic of producing an AllocId for a ConstAllocation to the Machine trait, and the implementation that Miri provides will only produce 16 AllocIds for each allocation. The cache is also keyed on the Instance that the const is evaluated in, so that equal consts evaluated in two functions will have disjoint base addresses.

r? RalfJung
@bors
Copy link
Contributor

bors commented Jan 24, 2024

⌛ Testing commit c8a675d with merge 5dd57b2...

@bors
Copy link
Contributor

bors commented Jan 24, 2024

💥 Test timed out

@bors 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 Jan 24, 2024
@RalfJung
Copy link
Member

@bors retry

@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 Jan 24, 2024
@bors
Copy link
Contributor

bors commented Jan 24, 2024

⌛ Testing commit c8a675d with merge cd6d8f2...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

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

@bors
Copy link
Contributor

bors commented Jan 24, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing cd6d8f2 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 24, 2024
@bors bors merged commit cd6d8f2 into rust-lang:master Jan 24, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 24, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cd6d8f2): 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)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
4.5% [4.4%, 4.6%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.9% [-4.4%, -1.3%] 2
All ❌✅ (primary) 0.5% [0.5%, 0.5%] 1

Cycles

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

Binary size

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

Bootstrap: 663.47s -> 663.194s (-0.04%)
Artifact size: 308.38 MiB -> 308.36 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

7 participants