Extend std.conv.parse for strings to full ieeeQuadruple precision and other fixes

[joakim-noah]

This pull was tested with ldc on linux/x64, Android/AArch64, and Android/ARM, exercising 80-bit, 128-bit, and 64-bit reals. The new decimal test was off by one bit on the first and last platforms, but strangely matched exactly for the most precise, AArch64. Putting this up early to see what the CI says on other platforms, will have also extended the hex parser if all goes well.

[dlang-bot]

Thanks for your pull request, @joakim-noah!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6633"

[joakim-noah]

I'm unsure why this was made to max out at 47 bits, worry that it might overflow 64-bit reals with their 53-bit mantissas, when assigned to real ldval below? Tested this on Android/ARM with long.max assigned to a 64-bit real, no problem there.

[joakim-noah]

Stop at the largest power of 10 that a ulong can hold.

[wilzbach]

I think it makes sense to add this as a comment in the source for future readers.

[joakim-noah]

This finally exercises the extended precision code with lsdec above, which codecov.io says wasn't covered.

[joakim-noah]

Added a WIP commit to extend the hex parser to ieeeQuadruple precision, worked fine on linux/x64 and Android/AArch64, running it through the CI now.

[joakim-noah]

@jpf91, why do all the bit-packing above only for hex strings? Instead, I commented it out and simply assigned the mantissa to the real, as done for decimal strings below. I ignore rounding for now, which is done separately for full 80-bit reals and 64-bit reals above: I'm sure we could add it back in later once we decide how to handle this.

Update: hmm, all the manual bit-packing and rounding seems to serve no use, all the same tests pass just by assigning the long mantissa to the real ldval. I tried a few tests locally and rounding seems to be done properly too, as all the CI tests passing now would indicate.

[joakim-noah]

The Jenkins and Circle build problems are unrelated.

[joakim-noah]

Merged the two commits extending the parser and simply got rid of all the bit-packing and rounding, that the hex parser alone was doing. Now that they're pretty similar, going to refactor them into a single parser for both hex and decimal strings.

[joakim-noah]

Refactored the floating-point string parser so hex and decimal formats use the same, more compact code path, probably less performant but I'm not sure that matters. I'll add some more tests and extend others and this should be done.

[joakim-noah]

Alright, this should be is ready to go, once we know it passes the CI. I looked at issue 8523, CTFE parsing of hex floating-point strings gets a little farther now, but still craps out because of the ldexp call at the end, lack of D source. I suppose that could be done manually, as done for powers of 10 just below, if someone wanted to finally fix that issue.

[joakim-noah]

The Jenkins failure is because tsv-utils checks for a different error message on bad input, I'll submit a pull to that repo to change that test once this pull is approved. Circle failure still unrelated.

[jondegenhardt]

The Jenkins failure is because tsv-utils checks for a different error message on bad input, I'll submit a pull to that repo to change that test once this pull is approved.

I was waiting for this to happen. That is, for exception text to change across compiler/phobos versions. I've finessed it so far, but it'll be more difficult in this case. The way the tsv-utils tests are setup they don't know about version, but I still need to test against different compiler versions. It may be necessary to add support for multiple compiler versions, which will make any PR a bit more complicated. And, I don't really want to drop this test either. If a change can't be made quickly enough, pulling tsv-utils out of project tester for a period would be an option. I'll have to handle it eventually.

Even though what tsv-utils is reporting is a change in the exception text, it looks like the text msg in the modified exception is a bug. From the text failure, it seems that a const char[] val consisting of characters "F2" is being called as val.to!double. This is failing, and the text added to the exception is:

no digits seen for input "[70, 50]".

This is likely generated from parse in conv.d. However, the original "F2" input is getting corrupted. The message should instead be reporting:

no digits seen for input "F2".

For my tools, including the input text in the exception message would be a benefit, but only if it's correct. What's being tested in this case is a command line call like:

$ tsv-filter --eq 2:1 input1.tsv

This tells tsv-filter to keep lines where field two is equal to one. However, the file has a header and values on the first line don't convert to numbers. So, the full interaction is:

$ tsv-filter --eq 2:1 input1.tsv
Error [tsv-filter]: Could not process line or field: no digits seen
  File: input1.tsv Line: 1
  Is this a header line? Use --header to skip.

It would certainly be an improvement if the output showed the value that failed to parse:

$ tsv-filter --eq 2:1 input1.tsv
Error [tsv-filter]: Could not process line or field: no digits seen for input "F2".
  File: input1.tsv Line: 1
  Is this a header line? Use --header to skip.

But, corrupted text would be confusing.

[jondegenhardt]

Refactored the floating-point string parser so hex and decimal formats use the same, more compact code path, probably less performant but I'm not sure that matters.

I believe one of the reasons that tsv-utils, especially tsv-filter and tsv-summarize, are so much faster than the competition is because string-to-double conversion is fast in D. These tools make large numbers of conversions in tight loops. So, I'd like to encourage performance testing these changes as part of PR testing and due diligence.

[joakim-noah]

It would certainly be an improvement if the output showed the value that failed to parse

Yes, I changed the parser to call the bailOut function for that error, which also prints the input but hasn't been amended for the representation call at function start, as done elsewhere in #5015. I'll add the necessary cast and let's see if that's better.

I'd like to encourage performance testing these changes as part of PR testing and due diligence

OK, I'll check.

[joakim-noah]

Added a commit to fix that error message and a doc comment, both regressed in #5015. Benchmarking yields inconsistent results depending on the platform and compiler, I used this slightly amended version of the gist used to benchmark #5015, comparing with stock phobos and after applying this pull:

import std.stdio;
import std.conv;
import std.datetime;

__gshared char[] test = ['1', '2', '3', '4', '5', '.', '6', '7', '8', '9'];
__gshared char[] test1 = ['0', 'x', '3', '4', '5', '.', 'F', 'A', 'P', '9'];

void main()
{
    enum n = 10_000_000;

    double result, result1;
    StopWatch sw;
    Duration sum;
    TickDuration last = TickDuration.from!"seconds"(0);

    foreach(i; 0 .. n)
    {
        sw.start();

        result = parse!double(test);
        result1 = parse!double(test1);

        sw.stop();

        test = ['1', '2', '3', '4', '5', '.', '6', '7', '8', '9'];
        test1 = ['0', 'x', '3', '4', '5', '.', 'F', 'A', 'P', '9'];
        auto time = sw.peek() - last;
        last = sw.peek();

        sum += time.to!Duration();
    }

    writeln("Total time: ", sum);
}

With ldc 1.11 master and the -O3 flag, I get a fastest time of 2.99 seconds in a linux/x64 VPS originally, fastest run of 4.59 secs after this pull. However, dmd with -O -inline consistently has this pull faster, best time of 3.42 secs for this pull, 4.49 secs without (not recompiling phobos there as I always do with ldc, just modifying the std.conv from the latest dmd 2.081.1 release, don't think that makes a difference). Android/ARM with ldc 1.11 has this pull slower, whereas Android/AArch64 with ldc 0.17 ltsmaster and this std.conv.parse copy-pasted in has this pull consistently faster.

If you'd like to use the above benchmark or some other one on other platforms and compare, let me know what you find.

[joakim-noah]

Some more data points: I tried running that benchmark on linux/x64 with only the first two commits from this pull, ie before the refactoring commit, got a best run of 3.34 secs with dmd 2.081.1 and 2.50 secs with ldc 1.11. With ldc 0.17 running natively on Android/AArch64, I get a best run of 6.61 s originally and a tie of 4.03 s both with and without the refactoring commit.

This implies the refactoring commit slightly slows down dmd on linux/x64 too, but has no effect on Android/AArch64 for some reason.

[jondegenhardt]

I should be able to plug this into my standard benchmarks, but it'll be a couple days before I get a chance to do so. Also, I should be able to change the tsv-utils tests, but again, it'll be a couple days before I get to it.

[jondegenhardt]

I ran my tsv-summarize and tsv-filter (numeric filter) benchmarks on MacOS, these are the ones using string-to-double conversion significantly (See: MacOS top-4 and Data files and Command Lines).

I built both current LDC and current LDC plus this PR and rebuilt both tools. Unfortunately wasn't able to use LTO and PGO, this would take a bit more work on my part. Both builds use -release -O3 -boundscheck=off. I ran each benchmark three times. I also include the times from the last tsv-utils prebuilt binaries which are built using LTO and PGO. The 3 times from each build are below. Times in seconds.

Benchmark	Tool	tsv-utils release binaries	current LDC master	current LDC master w/ PR 6633
Numeric row filter	tsv-filter	3.51, 3.65, 3.49	5.74, 5.73, 5.73	7.08, 7.05, 7.10
Summary Statistics	tsv-summarize	9.98, 9.97, 9.93	30.03, 30.05, 30.03	44.34, 43.37, 44.18

This shows a significant performance hit due to this PR. Clearly times all times are dramatically impacted by not using LTO and PGO in the builds. However, the delta from this PR is significant enough that it's unlikely to be made up by turning LTO and PGO on.

Note that the delta due to conversion by itself is percentage-wise significantly more than the deltas shown above. These programs spend a significant amount of time doing I/O and other computations, they are not just doing string conversions.

[joakim-noah]

Can you also try with just the first two commits from this pull applied, ie without the third refactoring commit? In my experience, that has actually been faster than stock std.conv.parse.

[jondegenhardt]

Here are the numbers with LTO & PGO. Quite a bit better than I expected, but still a hit to tsv-summarize. I ran each six times.

Benchmark	Tool	current LDC master	current LDC master w/ PR 6633
Numeric row filter	tsv-filter	3.61, 3.59, 3.61, 3.61, 3.62, 3.60	3.66, 3.66, 3.66, 3.66, 3.67, 3.67
Summary Statistics	tsv-summarize	11.93, 11.95, 11.97, 11.89, 11.91, 11.97	12.82, 12.79, 12.85, 12.94, 12.82, 12.82

About a 7% impact on tsv-summarize, and about 1% on tsv-filter.
Note: tsv-summarize has slowed down over recent LDC releases prior to this PR. The tsv-utils prebuilt binaries are build with LDC 1.5 and run the Summary Statistics test in about 9.93.

[jondegenhardt]

Sure, I'll try the first two commits and drop the third.

[jondegenhardt]

Yes, the first two commits are faster. It's very significant when not using LTO/PGO. It's also significant when using LTO/PGO. For tsv-summarize it's faster than stock by 12%, faster than the full PR by 18%. It'd be great if you can preserve this.

Times with -release -O3 -boundscheck=off, but no LTO, no PGO (3 runs):

Benchmark	Tool	current LDC master	current LDC master w/ PR 6633	current LDC master w/ PR 6633 first 2 commits
Numeric row filter	tsv-filter	5.75, 5.74, 5.73	7.10, 7.10, 7.07	5.69, 5.67, 5.68
Summary Statistics	tsv-summarize	30.07, 30.18, 30.03	44.14, 44.14, 44.17	29.51, 29.46, 29.52

Times with LTO & PGO (3 runs):

Benchmark	Tool	current LDC master	current LDC master w/ PR 6633	current LDC master w/ PR 6633 first 2 commits
Numeric row filter	tsv-filter	3.59, 3.61, 3.60	3.66, 3.67, 3.66	3.53, 3.54, 3.54
Summary Statistics	tsv-summarize	11.96, 11.93, 11.93	12.92, 12.89, 12.82	10.42, 10.44, 10.46

[joakim-noah]

OK, that coincides with my own microbenchmark results above. Honestly, the LTO/PGO results with the refactoring included are not much worse, I think the simplicity from refactoring is worth a possible 1-7% performance hit on certain platforms/compilers. However, I'll do whatever the consensus is from phobos reviewers, can simply drop the third refactoring commit if prefered.

[jondegenhardt]

The 1-7% with LTO/PGO is compared to current stock. However, the delta between the 2nd commit and full PR is 18-20% with LTO/PGO.

I'd certainly agree that my apps stress the string conversions more than most.

[joakim-noah]

Yes, the second commit speeds up the hex parser, by throwing out a bunch of manual bit-packing and rounding, and the third commit can slow both the hex/decimal parsers back down again, by combining both parsers into one code path, though that refactoring seems to have no effect on certain platforms/compilers, like ldc 0.17 for Android/AArch64.

[JohanEngelen]

My opinion on the performance aspect: the refactoring is something separate from the extension of functionality. So I'd remove that 3rd commit for now, and perhaps put up a second PR where the refactoring can be discussed in isolation. The performance hit is very large, and LTO/PGO are not always an option.
(it'd be good to add a comment in the code that it's a perf sensitive function, so that future changes check performance changes)
(btw, great that you got the perf boost in the first 2 commits!)

[jondegenhardt]

I agree with @JohanEngelen regarding splitting the refactoring from the functionality change.

[joakim-noah]

I investigated the performance drop from refactoring the parsers into one code path and tried to fix it by templatizing it, see last commit. I didn't bother formatting it because I just wanted to get the idea across, I don't know if it's idiomatic either. It seems to gain back the performance of the first two commits while not having the parser code replicated as before.

[jondegenhardt]

Thanks! It was that first switch statement I was looking at. From a review perspective, the first question I had was if this was a behavior change. I thought it might be part of "and other fixes" in the PR title.

At to the tsv-utils tests, Jenkins is broken and the tsv-utils tests are being aborted before being run, but recorded as success. If the error message has changed, they will start reporting failure at some point.

[joakim-noah]

The other fixes are broken out into separate commits, like adding tests or fixing doc comments. I haven't been able to load that Jenkins results webpage in awhile- always shows a blue line that goes almost to the right edge and freezes the last 10 times I tried- didn't investigate the tsv-utils build log back when it last loaded.

[wilzbach]

Looks like this pull is ready to go, the Jenkins issues with vibe-core appear unrelated.

Yes, please ignore all Jenkins failures. We're in the process of switching to Buildkite, but sadly their logs aren't public yet. We're on the promised list of the private beta for this, but until then the only way to see the logs is to login (that's why I sent an invite to both of you or ignore if you aren't interested).
Anyhow, as long as the Buildkite status check is green, there's nothing to worry about regarding the "Project Tester" ;-)

[jondegenhardt]

Anyhow, as long as the Buildkite status check is green, there's nothing to worry about regarding the "Project Tester" ;-)

Okay, then we do need to check the Buildkite logs. I was expecting the tsv-utils tests to fail due to the change in text generated in the error cases. Unless the latest update does not change the text in the exception thrown. I'll look at the Buildkite logs when I get a chance, probably within a day.

[wilzbach]

It could be something related to: dlang/ci#245 (I will trigger a rebuild once that's merged)

[jondegenhardt]

Yeah, looks like the tsv-utils tests aren't being run in current buildkite config. The tests finish in 0 seconds, which says they didn't run.

[joakim-noah]

Modified the refactoring to move this NaN check out of the switch statement I'd moved it to above before, which is in keeping with the way it was done before this pull, ie optimizing for the presumably more common non-NaN case by not checking for it until later.

[jondegenhardt]

fyi - For tsv-utils I've decided on an approach to support versioned test results. This is what I'll use to handle cases like this going forward. Shouldn't be take too long for me to get it in place, but I can't guarantee when it'll be done. Feel free to ping me if a status update is needed. At the moment this PR looks like it's also waiting on other reviews and potentially other buildkite project tests.

[joakim-noah]

@n8sh or @9il, this pull is ready, should be straightforward to review, particularly if you go commit by commit.

[9il]

Greate work. Would be awesome to see a new Mir library with BetterC analog of this PR

[wilzbach]

Just for other reviewers, the other buildkite failures were spurious and the tsv-filter one is the only one which looks permanent:

Testing /var/lib/buildkite-agent/builds/buildkite-agent-01-1/dlang/phobos/tsv-utils/bin/tsv-filter.dbg, output to latest_debug
Files tests/latest_debug/error_tests_1.txt and tests/gold/error_tests_1.txt differ
---> tsv-filter command line tests failed.
diff tests/latest_debug/error_tests_1.txt tests/gold/error_tests_1.txt
174c174
< Error [tsv-filter]: Could not process line or field: no digits seen for input "F2".
---
> Error [tsv-filter]: Could not process line or field: no digits seen
179c179
< Error [tsv-filter]: Could not process line or field: no digits seen for input "f2".
---
> Error [tsv-filter]: Could not process line or field: no digits seen
../makeapp.mk:53: recipe for target 'test-debug' failed
make[1]: *** [test-debug] Error 1
make[1]: Leaving directory '/var/lib/buildkite-agent/builds/buildkite-agent-01-1/dlang/phobos/tsv-utils/tsv-filter'
makefile:90: recipe for target 'tsv-filter' failed

[wilzbach]

As we now switched fully to Buildkite and the logs are still not public, I (as an experiment) created a dummy account with readonly permissions for those who haven't got an account at Buildkite yet:

user: dummy@dlang.io
password: dlangrocks

(one can't do much with this account, except for being a troll and changing the password. The only effect would be that dummy account can no longer be used.)

[joakim-noah]

@klickverbot or @ibuclaw, review needed: you guys did a lot of the work to get quadruple-precision working. This should finish it up, along with the initial commit in dlang/druntime#2257.

[ibuclaw]

Looks reasonable at a glance. Has the performance regression been sufficiently dealt with?

[joakim-noah]

Yes, see comment above, parsing should always be faster after this pull is applied now, which wasn't always the case with previous iterations of this PR.

[jondegenhardt]

The tsv-utils tests now pass. The version just released now recognizes both the old and new error message text as correct results.

[joakim-noah]

Thanks, much more involved diff than I thought, buildkite passing now.

OK, just need someone to approve now.

[jondegenhardt]

@joakim-noah You're welcome. A change I had to make eventually. It would have been an involved task for someone unfamiliar with the build setup.

[joakim-noah]

Rebased and made one small tweak, moved this check into parseDigits() so it becomes a static check, instead of being done at runtime before.

[joakim-noah]

Thought updating might get DAutoTest to work, but looks like it's just broken right now for every pull.