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

pull sipa wants to merge 1 commits into bitcoin:master from sipa:201804_noimbue changing 3 files +14 −28

sipa commented at 5:31 AM on April 13, 2018: member

And replace them with just hardcoded ISO8601 strings and gmtime_r.

Pointed out by @laanwj here: #12970 (comment)
in src/utiltime.cpp:83 in 3a3ea86ec7 outdated
```
  85 | -    ss << boost::posix_time::from_time_t(nTime);
  86 | -    return ss.str();
  87 | +    char buf[256];
  88 | +    struct tm time_struct;
  89 | +    time_t time_val = nTime;
  90 | +    gmtime_r(&time_val, &time_struct);
```
theuni commented at 5:40 AM on April 13, 2018:

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

sipa commented at 6:24 AM on April 13, 2018:

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

ken2812221 commented at 7:58 AM on April 13, 2018:

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

sipa commented at 8:51 AM on April 13, 2018:

@ken2812221 gmtime is not thread safe. We either user gmtime_r, or introduce a lock around the call.
fanquake added the label Refactoring on Apr 13, 2018
practicalswift commented at 5:42 AM on April 13, 2018: contributor

Concept ACK
laanwj commented at 6:27 AM on April 13, 2018: member
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 commented at 6:30 AM on April 13, 2018: member
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 commented at 6:37 AM on April 13, 2018: member

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: 3e8ac6af9a993e262d1160fb2e6e1e1f1d5d19f2

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.
Avoid std::locale/imbue in DateTimeStrFormat 1527015681
sipa commented at 8:37 AM on April 13, 2018: member

@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.
sipa force-pushed on Apr 13, 2018
laanwj commented at 12:20 PM on April 13, 2018: member

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 :+1: It's very straightforward.

tACK 152701568159915828613fa131e94e21337c0c2d
practicalswift commented at 12:29 PM on April 13, 2018: contributor

utACK 152701568159915828613fa131e94e21337c0c2d

in src/utiltime.cpp:82 in 1527015681

  89 | -
  90 |  std::string FormatISO8601DateTime(int64_t nTime) {
  91 | -    return DateTimeStrFormat("%Y-%m-%dT%H:%M:%SZ", nTime);
  92 | +    struct tm ts;
  93 | +    time_t time_val = nTime;
  94 | +    gmtime_r(&time_val, &ts);

laanwj commented at 12:51 PM on April 13, 2018:

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

jonasschnelli commented at 6:36 PM on April 13, 2018: contributor

utACK 152701568159915828613fa131e94e21337c0c2d
laanwj merged this on Apr 14, 2018
laanwj closed this on Apr 14, 2018
laanwj referenced this in commit dec61152d6 on Apr 14, 2018
sipsorcery commented at 1:22 AM on April 15, 2018: member

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.
laanwj referenced this in commit 826acc9a3d on Apr 26, 2018
PastaPastaPasta referenced this in commit e1967e42c4 on Apr 13, 2021
xdustinface referenced this in commit e182fa4ecb on Apr 14, 2021
PastaPastaPasta referenced this in commit c9ba65bb12 on Apr 14, 2021
PastaPastaPasta referenced this in commit 5e809e9e08 on Apr 14, 2021
PastaPastaPasta referenced this in commit 0ba44e7a6a on Apr 15, 2021
PastaPastaPasta referenced this in commit 9b11b3d8ef on Apr 17, 2021
PastaPastaPasta referenced this in commit bb1ae8452c on Apr 18, 2021
kittywhiskers referenced this in commit 318281c506 on Apr 23, 2021
random-zebra referenced this in commit 14060430d9 on May 5, 2021
UdjinM6 referenced this in commit 195fdfad5e on May 21, 2021
UdjinM6 referenced this in commit eac133cea4 on May 25, 2021
TheArbitrator referenced this in commit 7559d807f1 on Jun 4, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12970 logging: bypass timestamp formatting when not logging #13031 Fix for utiltime to compile with msvc.