Improve performance of JSON rendering

[SiarheiFedartsou]

Issue

I noticed that there are a lot of things can be optimized in JSON rendering. See comments in code(please read in enumeration order - will be easier to follow timing changes)

Tasklist

• CHANGELOG.md entry (How to write a changelog entry)
• update relevant Wiki pages
• add tests (see testing documentation)
• review
• adjust for comments
• cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

[SiarheiFedartsou]

0.

Baseline on my M1 Pro/32 gb ram:

[SiarheiFedartsou]

"string" here is std::stringstream ss; ... = ss.str()

[SiarheiFedartsou]

1.

First of all I noticed that for some reason we use completely different implementation for std::vector and std::ostream outputs. I unified it using std::ostream implementation as common one.

[SiarheiFedartsou]

2.

Then I decided to add Renderer specialization for std::string(i.e. stopped using std::stringstream and started writing directly to std::string) and started using it in benchmark:

[SiarheiFedartsou]

3.

Then I noticed that we spend quite a lot of time on escaping the strings which usually don't need to be escaped:

[SiarheiFedartsou]

4.

And finally I noticed that we are doing unneeded copy of the whole json here and it gave the most significant improvement:

[SiarheiFedartsou]

This JSON is just a copy-paste from one of the test cases.

[SiarheiFedartsou]

Started using std::string here as it is faster. osrm-routed uses std::vector.

[SiarheiFedartsou]

We probably can even try to go with std::vector here too(which is slightly faster according to benchmarks).

[SiarheiFedartsou]

We spend quite a lot on number formatting: I've tried to optimize it using fmt(which promises to have faster formatting than current algorithm), but I couldn't notice the difference.

[danpat]

Yeah - I optimized this part a long time ago - if you dig into this function, you'll notice we're already using a highly optimized floating point -> string rendering implementation, rather than just doing std::to_string(number).

Turning floats into strings is fairly expensive, which is why it still shows up in a profiler run. There are a lot of papers about number->string rendering performance, you can possibly find a better implementation than the one already here, but the gains over what we have will likely be small.

[SiarheiFedartsou]

Yes, but as I can see we currently use milo algorithm(did you modify it in some way btw?) and fmtlib author says their algorithm is even faster.

It seemed to be low-hanging fruit to use it here(we already depend on fmtlib anyway), but 100% agree - no reason to waste time on it since possible improvement is marginal.

[SiarheiFedartsou]

It seems we have UB in current implementation 🤔

https://github.com/Project-OSRM/osrm-backend/actions/runs/3160140786/jobs/5144255047

It seems we had no UBSAN alerts here before, because for std::vector ArrayRenderer was used, which used std::stringstream for float numbers rendering.

[SiarheiFedartsou]

Checked on separate PR that Renderer has this UB problem #6381
So it seems to be another reason to switch to fmt...

[SiarheiFedartsou]

5.

With fmt it feels as it is slightly faster(but hard to notice the difference without statistical tests - I am judging just by presence of < 10ms measurements) and no UBs at the same time :)

[SiarheiFedartsou]

Seems to be expected since we changed algorithm used for formatting.

[SiarheiFedartsou]

Tbh I do not understand why do we truncate it - I couldn't find anything about it in JSON spec. Just to save some bytes in corner cases?

[mjjbell]

I'd assume it's because none of the API responses can attribute anything meaningful to more decimal places, so it saves response payload space + improves readability.

[SiarheiFedartsou]

This copy is expensive...

[mjjbell]

Can we instead just search for the existence of a character that requires escaping?

[mjjbell]

Do we have a feel for how often OSRM responses do require escaping? I.e how representative are these benchmarks?

[SiarheiFedartsou]

Can we instead just search for the existence of a character that requires escaping?

We can, but current implementation allows to preallocate string for escaped string if we need it(because we know required size - see how else we use this size a couple of lines below). And also assuming that vast majority of strings don’t need to be escaped(I.e. don’t contain special characters) we will have to scan the whole string most of the times anyway.

Do we have a feel for how often OSRM responses do require escaping?

That’s the great question. I don’t have numbers, just a gut feeling… Will try to collect some statistics from real responses.

Btw this idea is inspired by implementation in https://github.com/nlohmann/json

[SiarheiFedartsou]

Actually probably might be a good idea to take huge real JSON response, convert it to our json:: objects using script and try to check some of optimizations on it.

[SiarheiFedartsou]

Well, actually I found 2 "sources" of escaped strings in responses.

1. Geometries(tbh I didn't know we can have slashes there and they appear quite often...)

But even though they appear quite often these strings are usually relatively big and it should allow us to preallocate string buffer for them and avoid expensive reallocations during escaping(but we have to scan string twice)...

2. Road names and pronunciations. I quickly hacked osrm-extract to count number of name and name:pronunciation values which need to be escaped in osm.pbf and got the following numbers for Croatia and Poland respectively:

After that I updated benchmark to use real output of OSRM(huge route from Portugal to Korea) and "played" a bit with this optimisation. And my conclusion is that it seems this optimisation is still useful on real JSONs, but improvement is barely noticeable.

Without this optimisation (i.e. just always call EscapeJSONString)

With optimisation

So I don't have strong opinion here: we can remove it(less code == more readable code) or leave it as is since it seems to make things slightly better.

WDYT?

[SiarheiFedartsou]

Can we instead just search for the existence of a character that requires escaping?

Decided to check this variant(i.e. without preallocation if string requires escaping) and it surprisingly even more better on this particular benchmark.

[SiarheiFedartsou]

And I decided to try the following trick:

        if (RequiresJSONStringEscaping(string.value))
        {
            std::string escaped;
            // just a guess that 16 bytes for escaped characters will be enough to avoid
            // reallocations
            escaped.reserve(string.value.size() + 16);
            EscapeJSONString(string.value, escaped);

            write(escaped);
        }
        else
        {
            write(string.value);
        }

And it seems to be even better:

[mjjbell]

The String performance is worse in the final one?
In any case, the deltas seem quite minor and the optimisation code is relatively small, so we can go with whichever is best.

[SiarheiFedartsou]

Hm, interesting, tbh I was paying attention to Vector only. Will try to re-check that and will make a final decision. 👍🏻

[SiarheiFedartsou]

After re-running it several times with and without this preallocation it is really hard to say where String is better, so I decided to not waste time on it and just leave existing implementation.

[mjjbell]

I'd assume it's because none of the API responses can attribute anything meaningful to more decimal places, so it saves response payload space + improves readability.

[SiarheiFedartsou]

We probably could return index from this function and start EscapeJSONString execution from it, but IMO it is overkill.