-
Notifications
You must be signed in to change notification settings - Fork 353
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
localtime_r: deduplicate timezone name allocation #4069
localtime_r: deduplicate timezone name allocation #4069
Conversation
@rustbot ready |
Thanks for the PR! However, I am afraid this is the wrong approach. I have added a comment in #4025 laying out the right way to do deduplication here.
Please write comments like this in a way that github recognizes so that issues get automatically closed. For instance: "Fixes #4025". |
Thank you, @RalfJung, for providing detailed implementation guidelines. I referred to your comment here: #4025 (comment). In the current implementation, I assumed that all I’ll begin working on the suggested guidelines, as they provide a solid starting point for refactoring the code. |
Concurrency is not even needed to cause problems. One could run the interpreter to completion once, and then afterwards start a second interpreter and run that, and your cache would try to use old pointers from the first interpreter. Generally, |
Hi @RalfJung, Completed implementing the changes requested in #4025. Current Status:
Question: Since this PR depends on the RUSTC changes and isn't currently buildable independently, would it make sense to:
Looking forward to your guidance on how to proceed. |
I'd say we land this in two stages -- rustc PR first, then this one. So don't worry about CI here for now. Just make sure that the rustc PR is ready and passes CI. :) |
Hi @RalfJung, Thank you for the guidance on the two-stage implementation approach. I've prepared the RUSTC PR with the following implementation details: Key Implementation Choices:
Please let me know if any of these choices need adjustment to better align with the requirements. I'm happy to make any necessary refinements. Looking forward to your review of the RUSTC PR before proceeding with the MIRI changes. |
Let's discuss that in the rustc PR. :) I added it to my review queue. (And btw, it's rustc, not RUSTC. And we usually write Miri, not MIRI. Not a big deal, just FYI :) |
Thankyou :) |
@rustbot ready |
…str, r=RalfJung Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… Fixes rust-lang/miri#4025 This PR introduces a new `allocate_bytes` function in InterpCx and refactors `allocate_str` to use it internally. This change improves memory allocation handling in the interpreter by: 1. Adding `allocate_bytes`: - Direct byte slice allocation support - Handles both mutable and immutable allocations - Maintains proper memory alignment and deduplication 2. Refactoring `allocate_str`: - Now uses `allocate_bytes` internally - Adds string-specific metadata handling - Preserves existing string allocation behavior This is part 1 of the planned changes to improve timezone string handling in Miri. A follow-up PR will update Miri's timezone logic to use this new allocation mechanism. Related: rust-lang/miri#4069
…str, r=RalfJung Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… Fixes rust-lang/miri#4025 This PR introduces a new `allocate_bytes` function in InterpCx and refactors `allocate_str` to use it internally. This change improves memory allocation handling in the interpreter by: 1. Adding `allocate_bytes`: - Direct byte slice allocation support - Handles both mutable and immutable allocations - Maintains proper memory alignment and deduplication 2. Refactoring `allocate_str`: - Now uses `allocate_bytes` internally - Adds string-specific metadata handling - Preserves existing string allocation behavior This is part 1 of the planned changes to improve timezone string handling in Miri. A follow-up PR will update Miri's timezone logic to use this new allocation mechanism. Related: rust-lang/miri#4069
…str, r=RalfJung Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… Fixes rust-lang/miri#4025 This PR introduces a new `allocate_bytes` function in InterpCx and refactors `allocate_str` to use it internally. This change improves memory allocation handling in the interpreter by: 1. Adding `allocate_bytes`: - Direct byte slice allocation support - Handles both mutable and immutable allocations - Maintains proper memory alignment and deduplication 2. Refactoring `allocate_str`: - Now uses `allocate_bytes` internally - Adds string-specific metadata handling - Preserves existing string allocation behavior This is part 1 of the planned changes to improve timezone string handling in Miri. A follow-up PR will update Miri's timezone logic to use this new allocation mechanism. Related: rust-lang/miri#4069
Rollup merge of rust-lang#133861 - shamb0:refactor_InterpCx_allocate_str, r=RalfJung Add allocate_bytes and refactor allocate_str in InterpCx for raw byte… Fixes rust-lang/miri#4025 This PR introduces a new `allocate_bytes` function in InterpCx and refactors `allocate_str` to use it internally. This change improves memory allocation handling in the interpreter by: 1. Adding `allocate_bytes`: - Direct byte slice allocation support - Handles both mutable and immutable allocations - Maintains proper memory alignment and deduplication 2. Refactoring `allocate_str`: - Now uses `allocate_bytes` internally - Adds string-specific metadata handling - Preserves existing string allocation behavior This is part 1 of the planned changes to improve timezone string handling in Miri. A follow-up PR will update Miri's timezone logic to use this new allocation mechanism. Related: rust-lang/miri#4069
@shamb0 I have synced Miri with the latest rustc. So please rebase this PR over the latest Miri master branch, run |
4745d81
to
7d874af
Compare
Hi @RalfJung, Thank you for the guidance! The local tests look good, and I believe the CI build should pass now. Once confirmed, we can proceed with the review and intake. Let me know if any further enhancements are needed. :) |
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Some minor comments for the tests.
The new tests have nothing to do with the deduplication change, right? That confused me for a bit.^^
Would it be worth adding a test that checks the deduplication? That should be fairly easy, just call localtime_r
again and check that the tm_zone pointer stays the same.
3e121f0
to
8e71c74
Compare
Hi @RalfJung,
Looking forward to your insights! |
Ah, right... so what the test would have to do is call |
8e71c74
to
df4d483
Compare
Hi @RalfJung, I’ve implemented the test scenario
--- tests/pass-dep/libc/libc-time.stdout
+++ <stdout output>
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 24, allocation id: a2863
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 31, allocation id: a3683
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 21, allocation id: a4120
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 10, allocation id: a4557
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 22, allocation id: a6400
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 19, allocation id: a6837
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 28, allocation id: a7274
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 6, allocation id: a7711
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 7, allocation id: a10898
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 1, allocation id: a11334
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 28, allocation id: a7274
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 6, allocation id: a7711
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 9, allocation id: a12517
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 31, allocation id: a3683
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 26, allocation id: a13328
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 2, allocation id: a13765
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 22, allocation id: a6400
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 13, allocation id: a14691
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 20, allocation id: a15127
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 22, allocation id: a6400
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 20, allocation id: a15127
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 9, allocation id: a12517
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 17, allocation id: a21807
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 21, allocation id: a4120
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 22, allocation id: a6400
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 3, allocation id: a22991
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 5, allocation id: a23428
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 13, allocation id: a14691
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 12, allocation id: a24238
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 31, allocation id: a3683
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 17, allocation id: a21807
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 18, allocation id: a25420
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 26, allocation id: a13328
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 15, allocation id: a26231
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 15, allocation id: a26231
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 0, allocation id: a27272
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 0, allocation id: a27272
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 24, allocation id: a2863
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 12, allocation id: a24238
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 3, allocation id: a22991
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 12, allocation id: a24238
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 5, allocation id: a23428
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 14, allocation id: a30178
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 22, allocation id: a6400
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 0, allocation id: a27272
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 23, allocation id: a31476
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 13, allocation id: a14691
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 13, allocation id: a14691
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 0, allocation id: a27272
+Allocating deduplicated bytes: [45, 48, 55, 0], salt: 26, allocation id: a13328
+Number of unique tm_zone pointers: 24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the point of test_localtime_r_verify_string_deduplication... that's a very complicated test but what does it test?
test_localtime_r_multiple_calls_deduplication looks great! However, please make the entire test cfg(any(...))
; there's no point in even running this test on targets that don't have the tm_zone field.
cfd6df9
to
332dc52
Compare
Hi @RalfJung, Thank you for the feedback! The function Currently, all tests in Thanks again for your suggestions. |
|
332dc52
to
8c5bc25
Compare
Thankyou :), I hope PR is ready for intake review 👍🏾 |
Remember to write this when the PR is ready :) |
tests/pass-dep/libc/libc-time.rs
Outdated
const TIME_SINCE_EPOCH: libc::time_t = 1712475836; | ||
let custom_time_ptr = &TIME_SINCE_EPOCH; | ||
let mut tm = libc::tm { | ||
// Helper function to create an empty tm struct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Helper function to create an empty tm struct. | |
/// Helper function to create an empty tm struct. |
///
indicates a doc comment that documents the following function.
Please also do that with the other similar comments in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I wrote this:
Please also do that with the other similar comments in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated doc comments for all newly introduced function headers.
tests/pass-dep/libc/libc-time.rs
Outdated
|
||
let mut unique_pointers = std::collections::HashSet::new(); | ||
|
||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this unsafe block smaller. Only the call to localtime_r
needs to be inside of it, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good One, Thankyou :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is not resolved yet, either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I modified the code, but I'm wondering why it hasn't been committed :)
for i in 0..NUM_CALLS {
let timestamp = TIME_SINCE_EPOCH_BASE + (i as libc::time_t * 3600); // Increment by 1 hour for each call
let mut tm: libc::tm = create_empty_tm();
let tm_ptr = unsafe { libc::localtime_r(×tamp, &mut tm) };
assert!(!tm_ptr.is_null(), "localtime_r failed for timestamp {timestamp}");
unique_pointers.insert(tm.tm_zone);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I can't help you with that -- you have to commit and push your changes as usual.
@rustbot author |
@rustbot ready |
No this is not ready yet, some of my comments from the previous round have not been resolved. |
Signed-off-by: shamb0 <[email protected]>
b2153b6
to
ab15d13
Compare
@rustbot ready |
Looks great, thanks. :) |
Fixes #4025
This PR improves timezone string handling in
localtime_r()
by utilizing the newly introducedallocate_bytes()
function from rust-lang/rust#133861. This change provides a more efficient approach to string deduplication and memory management.Changes implemented:
allocate_bytes()
inlocaltime_r()
:This is part 2 of the planned improvements for timezone handling:
allocate_bytes()
to InterpCxallocate_bytes()
Related: rust-lang/rust#133861