Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of JSON rendering #6380

Merged
merged 29 commits into from
Oct 3, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
463663b
wip
SiarheiFedartsou Sep 28, 2022
a9c72e3
Refactor json renderer to share implementation for vector and string
SiarheiFedartsou Sep 30, 2022
cb0af6b
Add string
SiarheiFedartsou Sep 30, 2022
4b0ea28
Merge branch 'master' into sf-json-perf
SiarheiFedartsou Sep 30, 2022
c784ff7
Optimize strings
SiarheiFedartsou Sep 30, 2022
b9957bb
Get rid of copy
SiarheiFedartsou Sep 30, 2022
3781068
add check
SiarheiFedartsou Sep 30, 2022
666ff1c
use std::string in node bindings
SiarheiFedartsou Sep 30, 2022
d2ce6d2
add comments
SiarheiFedartsou Sep 30, 2022
ca7e560
fix bug
SiarheiFedartsou Sep 30, 2022
05c6008
add comments
SiarheiFedartsou Sep 30, 2022
4f222be
add comments
SiarheiFedartsou Sep 30, 2022
889e4ce
add comments
SiarheiFedartsou Sep 30, 2022
87db480
add comments
SiarheiFedartsou Sep 30, 2022
258d656
fix bug
SiarheiFedartsou Oct 1, 2022
a4d1783
use fmt
SiarheiFedartsou Oct 1, 2022
ee7499d
use fmt
SiarheiFedartsou Oct 1, 2022
d033e3b
use fmt
SiarheiFedartsou Oct 1, 2022
d871e20
use fmt
SiarheiFedartsou Oct 1, 2022
bc6a4d8
add test
SiarheiFedartsou Oct 1, 2022
ae157de
add test
SiarheiFedartsou Oct 1, 2022
6a5b3f4
add test
SiarheiFedartsou Oct 1, 2022
50efe45
update changelog
SiarheiFedartsou Oct 1, 2022
04429d4
update
SiarheiFedartsou Oct 1, 2022
7430d62
optimize nodejs
SiarheiFedartsou Oct 1, 2022
03f9671
update
SiarheiFedartsou Oct 1, 2022
32deb11
update benchmark
SiarheiFedartsou Oct 2, 2022
cbc0868
update benchmark
SiarheiFedartsou Oct 2, 2022
f057ef7
update benchmark
SiarheiFedartsou Oct 3, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/osrm-backend.yml
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,7 @@ jobs:
pushd ${OSRM_BUILD_DIR}
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.

Baseline on my M1 Pro/32 gb ram:
initial

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

make --jobs=${JOBS} benchmarks
./src/benchmarks/alias-bench
./src/benchmarks/json-render-bench
./src/benchmarks/match-bench ../test/data/ch/monaco.osrm
./src/benchmarks/packedvector-bench
./src/benchmarks/rtree-bench ../test/data/monaco.osrm.ramIndex ../test/data/monaco.osrm.fileIndex ../test/data/monaco.osrm.nbg_nodes
Expand Down
154 changes: 66 additions & 88 deletions include/util/json_renderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,32 @@ namespace
constexpr int MAX_FLOAT_STRING_LENGTH = 256;
}

struct Renderer
template <typename Out> struct Renderer
{
explicit Renderer(std::ostream &_out) : out(_out) {}
explicit Renderer(Out &_out) : out(_out) {}

void operator()(const String &string) const
void operator()(const String &string)
{
out << "\"";
out << escape_JSON(string.value);
out << "\"";
write('"');
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.

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

// here we assume that vast majority of strings don't need to be escaped,
// so we check it first and escape only if needed
auto size = SizeOfEscapedJSONString(string.value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Screenshot 2022-10-02 at 22 37 42

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)...

  1. 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:

Screenshot 2022-10-02 at 22 10 58

Screenshot 2022-10-02 at 22 37 02

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)

Screenshot 2022-10-02 at 22 38 44

With optimisation

Screenshot 2022-10-02 at 22 38 23

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Screenshot 2022-10-02 at 23 10 59

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screenshot 2022-10-02 at 23 18 17

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (size == string.value.size())
{
write(string.value);
}
else
{
std::string escaped;
escaped.reserve(size);
EscapeJSONString(string.value, escaped);

write(escaped);
}
write('"');
}

void operator()(const Number &number) const
void operator()(const Number &number)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Screenshot 2022-09-30 at 18 33 04

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we have UB in current implementation 🤔
Screenshot 2022-09-30 at 18 36 56

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Screenshot 2022-10-01 at 21 35 10

{
char buffer[MAX_FLOAT_STRING_LENGTH] = {'\0'};
ieee754::dtoa_milo(number.value, buffer);
Expand All @@ -64,131 +78,95 @@ struct Renderer
}
++pos;
}
out << buffer;
write(buffer);
}

void operator()(const Object &object) const
void operator()(const Object &object)
{
out << "{";
write('{');
for (auto it = object.values.begin(), end = object.values.end(); it != end;)
{
out << "\"" << it->first << "\":";
write('\"');
write(it->first);
write("\":");
mapbox::util::apply_visitor(Renderer(out), it->second);
if (++it != end)
{
out << ",";
write(',');
}
}
out << "}";
write('}');
}

void operator()(const Array &array) const
void operator()(const Array &array)
{
out << "[";
write('[');
for (auto it = array.values.cbegin(), end = array.values.cend(); it != end;)
{
mapbox::util::apply_visitor(Renderer(out), *it);
if (++it != end)
{
out << ",";
write(',');
}
}
out << "]";
write(']');
}

void operator()(const True &) const { out << "true"; }
void operator()(const True &) { write("true"); }

void operator()(const False &) const { out << "false"; }
void operator()(const False &) { write("false"); }

void operator()(const Null &) const { out << "null"; }
void operator()(const Null &) { write("null"); }

private:
std::ostream &out;
void write(const std::string &str);
void write(const char *str);
void write(char ch);

private:
Out &out;
};

struct ArrayRenderer
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
vector uses the same implementation

template <> void Renderer<std::vector<char>>::write(const std::string &str)
{
explicit ArrayRenderer(std::vector<char> &_out) : out(_out) {}

void operator()(const String &string) const
{
out.push_back('\"');
const auto string_to_insert = escape_JSON(string.value);
out.insert(std::end(out), std::begin(string_to_insert), std::end(string_to_insert));
out.push_back('\"');
}
out.insert(out.end(), str.begin(), str.end());
}

void operator()(const Number &number) const
{
const std::string number_string = cast::to_string_with_precision(number.value);
out.insert(out.end(), number_string.begin(), number_string.end());
}
template <> void Renderer<std::vector<char>>::write(const char *str)
{
out.insert(out.end(), str, str + strlen(str));
}

void operator()(const Object &object) const
{
out.push_back('{');
for (auto it = object.values.begin(), end = object.values.end(); it != end;)
{
out.push_back('\"');
out.insert(out.end(), it->first.begin(), it->first.end());
out.push_back('\"');
out.push_back(':');
template <> void Renderer<std::vector<char>>::write(char ch) { out.push_back(ch); }

mapbox::util::apply_visitor(ArrayRenderer(out), it->second);
if (++it != end)
{
out.push_back(',');
}
}
out.push_back('}');
}
template <> void Renderer<std::ostream>::write(const std::string &str) { out << str; }

void operator()(const Array &array) const
{
out.push_back('[');
for (auto it = array.values.cbegin(), end = array.values.cend(); it != end;)
{
mapbox::util::apply_visitor(ArrayRenderer(out), *it);
if (++it != end)
{
out.push_back(',');
}
}
out.push_back(']');
}
template <> void Renderer<std::ostream>::write(const char *str) { out << str; }

void operator()(const True &) const
{
const std::string temp("true");
out.insert(out.end(), temp.begin(), temp.end());
}
template <> void Renderer<std::ostream>::write(char ch) { out << ch; }

void operator()(const False &) const
{
const std::string temp("false");
out.insert(out.end(), temp.begin(), temp.end());
}
template <> void Renderer<std::string>::write(const std::string &str) { out += str; }

void operator()(const Null &) const
{
const std::string temp("null");
out.insert(out.end(), temp.begin(), temp.end());
}
template <> void Renderer<std::string>::write(const char *str) { out += str; }

private:
std::vector<char> &out;
};
template <> void Renderer<std::string>::write(char ch) { out += ch; }

inline void render(std::ostream &out, const Object &object)
{
Value value = object;
mapbox::util::apply_visitor(Renderer(out), value);
Renderer renderer(out);
renderer(object);
}

inline void render(std::string &out, const Object &object)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

string added

{
Renderer renderer(out);
renderer(object);
}

inline void render(std::vector<char> &out, const Object &object)
{
Value value = object;
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4.

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

mapbox::util::apply_visitor(ArrayRenderer(out), value);
Renderer renderer(out);
renderer(object);
}

} // namespace json
Expand Down
30 changes: 25 additions & 5 deletions include/util/string_util.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,32 @@ template <int length, int precision> char *printInt(char *buffer, int value)
return buffer;
}

inline std::string escape_JSON(const std::string &input)
inline size_t SizeOfEscapedJSONString(const std::string &string)
{
size_t size = 0;
for (const char letter : string)
{
switch (letter)
{
case '\\':
case '"':
case '/':
case '\b':
case '\f':
case '\n':
case '\r':
case '\t':
size += 2;
break;
default:
size += 1;
}
}
return size;
}

inline void EscapeJSONString(const std::string &input, std::string &output)
{
// escape and skip reallocations if possible
std::string output;
output.reserve(input.size() + 4); // +4 assumes two backslashes on avg
for (const char letter : input)
{
switch (letter)
Expand Down Expand Up @@ -96,7 +117,6 @@ inline std::string escape_JSON(const std::string &input)
break;
}
}
return output;
}

inline std::size_t URIDecode(const std::string &input, std::string &output)
Expand Down
15 changes: 15 additions & 0 deletions src/benchmarks/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,18 @@ target_link_libraries(match-bench
${TBB_LIBRARIES}
${MAYBE_SHAPEFILE})

add_executable(json-render-bench
EXCLUDE_FROM_ALL
json_render.cpp
$<TARGET_OBJECTS:UTIL>)

target_link_libraries(json-render-bench
osrm
${BOOST_BASE_LIBRARIES}
${CMAKE_THREAD_LIBS_INIT}
${TBB_LIBRARIES}
${MAYBE_SHAPEFILE})

add_executable(alias-bench
EXCLUDE_FROM_ALL
${AliasBenchmarkSources}
Expand All @@ -41,6 +53,8 @@ target_link_libraries(alias-bench
${TBB_LIBRARIES}
${MAYBE_SHAPEFILE})



add_executable(packedvector-bench
EXCLUDE_FROM_ALL
${PackedVectorBenchmarkSources}
Expand All @@ -58,4 +72,5 @@ add_custom_target(benchmarks
rtree-bench
packedvector-bench
match-bench
json-render-bench
alias-bench)
Loading