[vm/runtime] Call tzset() before localtime_r().

[JesseRiemens]

Relevant info in #60191 (And duplicate #61303)

POSIX doesn't guarantee that the timezone is set correctly before calling localtime_r(), so we need to call tzset() first to ensure that the timezone information is up-to-date.

As the glibc manual states:

According to POSIX.1-2001, localtime() is required to behave as though tzset(3) was called, while localtime_r() does not
have this requirement. For portable code, tzset(3) should be called before localtime_r().

• Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.

Note that this repository uses Gerrit for code reviews. Your pull request will be automatically converted into a Gerrit CL and a link to the CL written into this PR. The review will happen on Gerrit but you can also push additional commits to this PR to update the code review.

[copybara-service]

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/444983

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[mraleph]

Do you know what is the cost associated with calling tzset here? I would imagine it does a bunch of stuff to figure out the system timezone, how much does this add on top of what localtime_r costs?

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[JesseRiemens]

Do you know what is the cost associated with calling tzset here? I would imagine it does a bunch of stuff to figure out the system timezone, how much does this add on top of what localtime_r costs?

I made a quick benchmark that does ten runs of 10 million DateTime.now() calls, and the results are below:

The modded version, with the tzset() call:

~/Applications/dart-sdk-test/dart-modified/sdk/out/ReleaseX64/dart-sdk/bin/dart run ./bin/time_benchmark.dart  
Total time taken for 10000000 iterations: 345942 microseconds
Average time taken: 0.0345942 microseconds
Total time taken for 10000000 iterations: 345456 microseconds
Average time taken: 0.0345456 microseconds
Total time taken for 10000000 iterations: 387595 microseconds
Average time taken: 0.0387595 microseconds
Total time taken for 10000000 iterations: 344673 microseconds
Average time taken: 0.0344673 microseconds
Total time taken for 10000000 iterations: 343620 microseconds
Average time taken: 0.034362 microseconds
Total time taken for 10000000 iterations: 344166 microseconds
Average time taken: 0.0344166 microseconds
Total time taken for 10000000 iterations: 339798 microseconds
Average time taken: 0.0339798 microseconds
Total time taken for 10000000 iterations: 337510 microseconds
Average time taken: 0.033751 microseconds
Total time taken for 10000000 iterations: 337566 microseconds
Average time taken: 0.0337566 microseconds
Total time taken for 10000000 iterations: 340555 microseconds
Average time taken: 0.0340555 microseconds
Total time taken for all iterations: 3466881 microseconds
Average time taken per iteration: 0.03466881 microseconds

The non-modified version:

~/Applications/dart-sdk-test/dart-without-mod/sdk/out/ReleaseX64/dart-sdk/bin/dart run ./bin/time_benchmark.dart
Total time taken for 10000000 iterations: 344923 microseconds
Average time taken: 0.0344923 microseconds
Total time taken for 10000000 iterations: 350685 microseconds
Average time taken: 0.0350685 microseconds
Total time taken for 10000000 iterations: 355197 microseconds
Average time taken: 0.0355197 microseconds
Total time taken for 10000000 iterations: 338760 microseconds
Average time taken: 0.033876 microseconds
Total time taken for 10000000 iterations: 338500 microseconds
Average time taken: 0.03385 microseconds
Total time taken for 10000000 iterations: 339348 microseconds
Average time taken: 0.0339348 microseconds
Total time taken for 10000000 iterations: 338644 microseconds
Average time taken: 0.0338644 microseconds
Total time taken for 10000000 iterations: 345835 microseconds
Average time taken: 0.0345835 microseconds
Total time taken for 10000000 iterations: 344627 microseconds
Average time taken: 0.0344627 microseconds
Total time taken for 10000000 iterations: 339836 microseconds
Average time taken: 0.0339836 microseconds
Total time taken for all iterations: 3436355 microseconds
Average time taken per iteration: 0.03436355 microseconds

That's 0.03466881 with mod vs 0.03436355 without mod;
That's 0.9% (1.0089) increase.

If you have any improvement to my benchmark method, please let me know.
It seems like the difference compared to the rest of the overhead is very small.

Edit: It actually looks like after repeated testing that the modified build is faster often, and I can't understand why.

[mraleph]

I think this looks good to me. Could you apply the same patch to os_android (I assume it has the same problem?) and then I can land it.

[JesseRiemens]

If I'm correct, this fix was already applied on Android and MacOS but reverted for all three platforms for performance issue reasons on Android:
#53276

I could create that patch no issue, but I don't have a clue how I would be able to performance test it for Android. I do have a Mac mini m1 here, so if it's required, I could do the same test as I did for Linux.

[mraleph]

@JesseRiemens thanks for the link. It seems to have been reverted because there were some TSAN failures. Though there is no information on what exactly was failing and I don't see any warnings around multithreading in tzset man pages. I will add a TSAN bot to the CL to test.

Anyway, ignore my comment about Android because apparently they have fixed in Android O - localtime_r behaves as if it calls tzset, so we don't need to call it explicitly.

[mraleph]

Could you maybe add the following to os_android.cc before localtime_r? So that we don't ask this question next time:

// No need to call tzset() before localtime_r() because bionic 
// will handle timezone changes for us (starting from Android O).
// See https://android.googlesource.com/platform/bionic/+/ea87716696bf635706b6f3fa56b8a145add83aff

[JesseRiemens]

Done! I've added the comment in os_android.cc.

Just tested on my M1 mini, it doesn't seem to have this issue.

os_android.cc already has the following done:

void OS::Init() {
  // Calling tzset() is only necessary in Android API version 25 or earlier.
  if (android_get_device_api_level() < 26) {
    // In API version 25, calling tzset() results in a ~0.5% increase in
    // Flutter startup latency. In API version 31, calling tzset() results in
    // a >25% increase in startup latency.
    tzset();
  }
}

So we accept the tradeoff that for older Android versions, we only initialize the timezone on startup and don't update it runtime so we don't have a slower DateTime.now() call on newer platforms?

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[mraleph]

Could you apply the following patch on top of your change:

diff --git a/build/sanitizers/tsan_suppressions.cc b/build/sanitizers/tsan_suppressions.cc
index 9a3fb8764dd..62634cf9262 100644
--- a/build/sanitizers/tsan_suppressions.cc
+++ b/build/sanitizers/tsan_suppressions.cc
@@ -260,6 +260,10 @@ char kTSanDefaultSuppressions[] =
 // False positive in libc's tzset_internal, http://crbug.com/379738.
 "race:tzset_internal\n"

+// Suppression above does not work if we don't properly symbolize
+// tzset_internal frame. Suppress the closest caller.
+"race:dart::DN_HelperDateTime_timeZoneOffsetInSeconds\n"
+
 // http://crbug.com/380554
 "deadlock:g_type_add_interface_static\n"

This should fix TSAN breakage (which is a false positive).

[JesseRiemens]

Done!

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[JesseRiemens]

It seems this doesn't satisfy LUCI, anything else we could try?

[mraleph]

I am working on TSAN stuff here: https://dart-review.googlesource.com/c/sdk/+/445941, turns out all this code was just dead. I will ping here once I figure out the best shape of it. Sorry for the trouble :)

[mraleph]

I have sent https://dart-review.googlesource.com/c/sdk/+/445941 for review. Once that lands I will ping you here and you should be able to rebase your change (dropping changes to build/sanitizers/tsan_suppressions.cc which I have included into my CL). After that we should be able to land the change.

I also noticed that presubmit checking is not happy because the code is not formatted. You should run git cl format to reformat the code before uploading (this requires depot_tools to be in the path).

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/444983 has been updated with the latest commits from this pull request.

[mraleph]

Thank you for the contribution. The patch has landed!