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

JsonpUtils#objectParser toString roundtrip to deserialise object uses ~30%more CPU #471

Closed
pchudzik opened this issue Dec 27, 2022 · 2 comments · Fixed by #489
Closed

JsonpUtils#objectParser toString roundtrip to deserialise object uses ~30%more CPU #471

pchudzik opened this issue Dec 27, 2022 · 2 comments · Fixed by #489
Assignees
Labels
Category: Bug Something isn't working

Comments

@pchudzik
Copy link

Java API client version

<= 7.17

Java version

17

Elasticsearch Version

7.17 and 8.5

Problem description

Hey

I'm in the process of migrating from the old elastic search client that has been deprecated.

While testing we've noticed higher CPU load when running servers.

From observations I'd say ~30% higher cpu load to what we have with deprecated client.
If we decide to pull the trigger on migration it'll mean we'll have to run more replicas of the service which is talking to elastic and it means more $$$ we'll have to spend on infrastructure.

We use multi search requests which I think is important at this point as running regular search requests has no performance penalty.

Looking at profiler cpu sampling I've noticed that around 30% of deserialising multi search response is spend in co.elastic.clients.json.JsonpUtils#objectParser and current implementation of this method is some kind of a workaround put in there around one year ago:

String strObject = object.toString();
return mapper.jsonProvider().createParser(new StringReader(strObject));

This is not very CPU friendly (and as a result cost effective implementation).

I'm wondering if this workaround is still necessary or if there is any better way of doing this now.

I'll be happy to do some testing but at this point I think I know too little about json providers, factories, parsers, mappers and other things that are going on with json serialisation and deserialisation to be of any real help.

@swallez
Copy link
Member

swallez commented Jan 16, 2023

Thanks for reporting this issue. This object parser used when we have to perform some look ahead in the JSON stream, which happens on two occasions:

  • for polymorphic types that are distinguished by their type property (i.e. a property inside the JSON object)
  • for some types with variants that can be disambiguated by looking at the structure of the JSON objects. The variants do not have the same properties, and inspecting the properties allows knowing what is the actual variant that has to be deserialized.

The JSON-P API doesn't offer a way to buffer JSON events and replay them, so this was an admittedly costly way to achieve this, but we kept it as the objects to which it applies are generally small. Except indeed for the multisearch response that can be a pathological case for the 2nd form (type with variants), as every member of the response can be either a search result or an error. We therefore have to peek into each response element to know the type of the item.

We've started the work on this issue so that we don't have to use this costly approach, at least for multisearch responses.

@swallez
Copy link
Member

swallez commented Jan 18, 2023

This has been fixed in #489, currently for the JacksonJsonpMapper, which is what is generally used. The costly workaround has been eliminated entirely and the deserialization now does the minimal amount of look ahead/buffering necessary to distinguish between a result item and an error item.

In practical terms for msearch, this means just reading and buffering the first property name of response items, which is either took for successful results and error for error items.

This will be released in 7.17.9 and 8.6.1 later this month, and we're interested in your feedback on the performance improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants