Avoid std::locale/imbue madness in DateTimeStrFormat #12973

sipa · 2018-04-13T05:31:55Z

And replace them with just hardcoded ISO8601 strings and gmtime_r.

Pointed out by @laanwj here: #12970 (comment)

theuni · 2018-04-13T05:40:12Z

src/utiltime.cpp

+    char buf[256];
+    struct tm time_struct;
+    time_t time_val = nTime;
+    gmtime_r(&time_val, &time_struct);


Is this portable? Afiak all of the madness is usually an abstraction of these variants.

Well at least this code doesn't need any allocations.

Why don't just use gmtime? It's more portable.

@ken2812221 gmtime is not thread safe. We either user gmtime_r, or introduce a lock around the call.

practicalswift · 2018-04-13T05:42:10Z

Concept ACK

laanwj · 2018-04-13T06:27:41Z

A potential problem is that strftime depends on the locale, just like many other C string formatting functions, which has made us avoid them in general. From the developer notes:

Use std::string, avoid C string manipulation functions

Rationale: C++ string handling is marginally safer, less scope for buffer overflows and surprises with \0 characters. Also some C string manipulations tend to act differently depending on platform, or even the user locale

The imbue stuff forces using the 'C' locale, I'm afraid if we want to use C functions we'd need a C equivalent of that and that's probably just as bad.

laanwj · 2018-04-13T06:30:33Z

FWIW it doesn't need full strftime support. The only datetime formats left in non-test code are ISO8601:

src/utiltime.cpp:    return DateTimeStrFormat("%Y-%m-%dT%H:%M:%SZ", nTime);
src/utiltime.cpp:    return DateTimeStrFormat("%Y-%m-%d", nTime);
src/utiltime.cpp:    return DateTimeStrFormat("%H:%M:%SZ", nTime);

Another potential simplification could be around that they all deal with UTC, and have UNIX epoch format as input.

Edit: so to be clear, big concept ACK on getting rid of this madness, but let's think about the replacement a bit. Maybe there's something like tinyformat.h that we can just import from somewhere...

laanwj · 2018-04-13T06:37:07Z

The imbue stuff forces using the 'C' locale, I'm afraid if we want to use C functions we'd need a C equivalent of that and that's probably just as bad.

A reminder that this is where we're coming from: 3e8ac6a

Edit: Thinking about it further - it becomes less clear to me whether the slowness is due to our own code, or due to formatting, or something else. After all, the speed seems to depend on the operating system. Maybe the slowdown (fixed by #12970) is due to barraging the kernel with "get current time" system calls. This is fairly fast on Linux - due to vdso user-space implementation of some syscalls - but not on windows or other OSes.

sipa · 2018-04-13T08:37:14Z

@laanwj Thanks, you're right that this approach ignores the reasons why the old code existed in the first place. I've replaced it with just hardcoded ISO8601 format strings... please let me know if you think this is too NIH.

Perhaps this is overkill as it may not help for performance. I remember we've had multiple horror stories in the past with this code though; I'd prefer to get rid of it.

laanwj · 2018-04-13T12:20:31Z

I've replaced it with just hardcoded ISO8601 format strings... please let me know if you think this is too NIH.

No I think this is great 👍 It's very straightforward.

tACK 1527015

practicalswift · 2018-04-13T12:29:28Z

utACK 1527015

laanwj · 2018-04-13T12:51:07Z

src/utiltime.cpp

-    return DateTimeStrFormat("%Y-%m-%dT%H:%M:%SZ", nTime);
+    struct tm ts;
+    time_t time_val = nTime;
+    gmtime_r(&time_val, &ts);


I'm happy that this works on windows as-is as well!

jonasschnelli · 2018-04-13T18:36:22Z

utACK 1527015

@laanwj

1527015 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) Pull request description: And replace them with just hardcoded ISO8601 strings and `gmtime_r`. Pointed out by @laanwj here: #12970 (comment) Tree-SHA512: a459758b42ca56f8462115aefe8e6377c1319fce509ea64dbb767f3f087c9b848335954cb684e5896c38008847684045505a3e1559fb3e83b8e80e10b003d1e7

sipsorcery · 2018-04-15T01:22:59Z

As an fyi Windows doesn't come with gmtime_r so this PR broke the msvc build.

I'd say there's an easy enough workaround and will look into it.

abd58a2 Fix for utiltime to compile with msvc. (Aaron Clauson) Pull request description: This PR allows utiltime.cpp to compile with msvc after the changes introduced in #12973. Tree-SHA512: 7233b1c23400bf19aef2fcb6168009ef58b9e7f8e49c46d8cf9d04394091f370e39496d24ca00b294c4164bcfc04514e329bf6bb05169406c34ce7cd8c382565

@laanwj

…rmat 1527015 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) Pull request description: And replace them with just hardcoded ISO8601 strings and `gmtime_r`. Pointed out by @laanwj here: bitcoin#12970 (comment) Tree-SHA512: a459758b42ca56f8462115aefe8e6377c1319fce509ea64dbb767f3f087c9b848335954cb684e5896c38008847684045505a3e1559fb3e83b8e80e10b003d1e7

@laanwj

…rmat 1527015 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) Pull request description: And replace them with just hardcoded ISO8601 strings and `gmtime_r`. Pointed out by @laanwj here: bitcoin#12970 (comment) Tree-SHA512: a459758b42ca56f8462115aefe8e6377c1319fce509ea64dbb767f3f087c9b848335954cb684e5896c38008847684045505a3e1559fb3e83b8e80e10b003d1e7

@laanwj

…rmat 1527015 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) Pull request description: And replace them with just hardcoded ISO8601 strings and `gmtime_r`. Pointed out by @laanwj here: bitcoin#12970 (comment) Tree-SHA512: a459758b42ca56f8462115aefe8e6377c1319fce509ea64dbb767f3f087c9b848335954cb684e5896c38008847684045505a3e1559fb3e83b8e80e10b003d1e7

@laanwj

…rmat 1527015 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) Pull request description: And replace them with just hardcoded ISO8601 strings and `gmtime_r`. Pointed out by @laanwj here: bitcoin#12970 (comment) Tree-SHA512: a459758b42ca56f8462115aefe8e6377c1319fce509ea64dbb767f3f087c9b848335954cb684e5896c38008847684045505a3e1559fb3e83b8e80e10b003d1e7

f3f3d6c util: fix compilation with mingw-w64 7.0.0 (Fuzzbawls) f24da7d util: Avoid potential uninitialized read in FormatISO8601DateTime (Fuzzbawls) 120a12a util: Add type safe GetTime (Fuzzbawls) be48766 Fix for utiltime to compile with msvc. (Aaron Clauson) 2833406 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) 3331c25 Format automatic wallet backup file names in ISO 8601 basic format (Fuzzbawls) c32a208 Format timestamps using ISO 8601 formatting (e.g. "2018-02-28T12:34:56Z") (practicalswift) f8bd173 [logging] log system time and mock time (John Newbery) Pull request description: This is a backport of a collection of upstream PRs to bring our time utility more up to date. The following upstream PRs are included: - bitcoin#10383 - bitcoin#12567 - bitcoin#12973 - bitcoin#13031 - bitcoin#16046 - bitcoin#18162 - bitcoin#18358 The two user-facing notable changes here have been documented in the release notes template. I've also connected the functionality that `-logtimemicros` was supposed to provide. ACKs for top commit: furszy: code review ACK f3f3d6c random-zebra: utACK f3f3d6c after rebase, and merging... Tree-SHA512: 64f9cc1f7fc65c192f3b3ab869c057a81e007cf0fae82ecd23993c6b51830e4bc80656c699ba40bb8e019115a24d1ea88a618be0c0d7112749d9ce125d62ce44

abd58a2 Fix for utiltime to compile with msvc. (Aaron Clauson) Pull request description: This PR allows utiltime.cpp to compile with msvc after the changes introduced in bitcoin#12973. Tree-SHA512: 7233b1c23400bf19aef2fcb6168009ef58b9e7f8e49c46d8cf9d04394091f370e39496d24ca00b294c4164bcfc04514e329bf6bb05169406c34ce7cd8c382565

theuni reviewed Apr 13, 2018

View reviewed changes

fanquake added the Refactoring label Apr 13, 2018

Avoid std::locale/imbue in DateTimeStrFormat

1527015

sipa force-pushed the 201804_noimbue branch from 3a3ea86 to 1527015 Compare April 13, 2018 08:37

laanwj reviewed Apr 13, 2018

View reviewed changes

laanwj merged commit 1527015 into bitcoin:master Apr 14, 2018

sipsorcery mentioned this pull request Apr 19, 2018

Fix for utiltime to compile with msvc. #13031

Merged

laanwj mentioned this pull request Apr 23, 2018

build: Add linter checking for accidental introduction of locale dependence #13041

Merged

ken2812221 mentioned this pull request Sep 10, 2018

bug in DateTimeStrFormat? #5631

Closed

Fuzzbawls mentioned this pull request Apr 8, 2021

[Util] Time utility updates PIVX-Project/PIVX#2300

Merged

xdustinface added a commit to xdustinface/dash that referenced this pull request Apr 14, 2021

Fix linter for bitcoin#12973

e182fa4

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 14, 2021

Fix linter for bitcoin#12973

c9ba65b

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 14, 2021

Fix linter for bitcoin#12973

5e809e9

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2021

Fix linter for bitcoin#12973

0ba44e7

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid std::locale/imbue madness in DateTimeStrFormat #12973

Avoid std::locale/imbue madness in DateTimeStrFormat #12973

Uh oh!

sipa commented Apr 13, 2018 •

edited

Loading

Uh oh!

theuni Apr 13, 2018

Uh oh!

sipa Apr 13, 2018

Uh oh!

ken2812221 Apr 13, 2018

Uh oh!

sipa Apr 13, 2018

Uh oh!

practicalswift commented Apr 13, 2018

Uh oh!

laanwj commented Apr 13, 2018

Uh oh!

laanwj commented Apr 13, 2018 •

edited

Loading

Uh oh!

laanwj commented Apr 13, 2018 •

edited

Loading

Uh oh!

sipa commented Apr 13, 2018 •

edited

Loading

Uh oh!

laanwj commented Apr 13, 2018 •

edited

Loading

Uh oh!

practicalswift commented Apr 13, 2018

Uh oh!

laanwj Apr 13, 2018 •

edited

Loading

Uh oh!

jonasschnelli commented Apr 13, 2018

Uh oh!

sipsorcery commented Apr 15, 2018

Uh oh!

Uh oh!

Avoid std::locale/imbue madness in DateTimeStrFormat #12973

Avoid std::locale/imbue madness in DateTimeStrFormat #12973

Uh oh!

Conversation

sipa commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

sipa Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

sipa Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Apr 13, 2018

Uh oh!

laanwj commented Apr 13, 2018

Uh oh!

laanwj commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Apr 13, 2018

Uh oh!

laanwj Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Apr 13, 2018

Uh oh!

sipsorcery commented Apr 15, 2018

Uh oh!

Uh oh!

sipa commented Apr 13, 2018 •

edited

Loading

laanwj commented Apr 13, 2018 •

edited

Loading

laanwj commented Apr 13, 2018 •

edited

Loading

sipa commented Apr 13, 2018 •

edited

Loading

laanwj commented Apr 13, 2018 •

edited

Loading

laanwj Apr 13, 2018 •

edited

Loading