Improve exception handling for JsonXContentParser

[smalyshev]

Ensures that all exception handling in the parser follows single code path and that JSON exceptions are handled in one place.

[rjernst]

Seems like this could possibly have an impact on json parsing speed due to introducing lambdas at every token event. Is the lambda needed?
cc @original-brownbear

[smalyshev]

@rjernst Thanks, I did not consider the performance aspect, I'll look into that. The lambda is strictly speaking not necessary, that part was added to avoid copypasting try/catch all over, but if it kills the performance then I can go back to copypasting.

[original-brownbear]

Yea we can't do this, this is going to annihilate performance I would expect. We have some jmh benchmarks around mapper parsing that will probably show this quite clearly.

[smalyshev]

I run FilterContentBenchmark and don't see any substantial difference there (in fact, majority of runs were faster but not sure it means anything - probably just jitter) - is it the right one to use? I put the results here: https://docs.google.com/spreadsheets/d/1JxHtSVzgfcOCZVAVHuIj1NmkvUvZKRcN1gVCChXz3xo/edit in case it's useful. I'd expect JIT to unroll the statics and lambdas but I'm not sure how smart it is.

[original-brownbear]

@smalyshev Does that benchmark even exercise this code path much? Almost looks like it just delegates to com.fasterxml.jackson.core.JsonGenerator#copyCurrentStructure? Also I'd expect the cost in that one to mainly live with the builder side of things, not the parsing?

I'd expect org.elasticsearch.benchmark.index.mapper.BeatsMapperBenchmark to show a regression (but run it on a clean machine, not a laptop ... at least for me results on a MacBook or similar are just all over in the JMH runs).

[smalyshev]

@original-brownbear OK I'll try BeatsMapperBenchmark on a VM and see what happens.

[smalyshev]

@original-brownbear So this is what I get on the Google VM:
from main:

BeatsMapperBenchmark.benchmarkParseKeywordFields  1600172297  avgt    5  19035.809 ± 63.917  ns/op
BeatsMapperBenchmark.benchmarkParseKeywordFields  1600172297  avgt    5  19312.652 ± 82.225  ns/op
BeatsMapperBenchmark.benchmarkParseKeywordFields  1600172297  avgt    5  19097.191 ± 168.124  ns/op

with the patch

BeatsMapperBenchmark.benchmarkParseKeywordFields  1600172297  avgt    5  18866.318 ± 190.904  ns/op
BeatsMapperBenchmark.benchmarkParseKeywordFields  1600172297  avgt    5  18837.553 ± 221.828  ns/op
BeatsMapperBenchmark.benchmarkParseKeywordFields  1600172297  avgt    5  19106.008 ± 160.673  ns/op

I'm not sure why the patch gets a tiny bit faster results, but it seems to be all within the error bars. Any other suggestions about how I could test it?

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[original-brownbear]

@smalyshev even if you cannot show a regression in a microbenchmark. I would strongly discourage this kind of change. It might be that the compiler is able to inline the whole thing. But also it could be that for some percentage of runs It won't be able to do so (JIT is not deterministic at all and you might even be able to reproduce this fact by rerunning the benchmark a couple times in fresh JVMs).
If we want to make a change to this code, its keep it cleanly procedural IMO.

I also gave that benchmark a spin and it's not a good reproducer sadly because it mostly goes through the dot expanding parser which has so much overhead that it drowns out the overhead here. Sorry for the inaccurate suggestion, I couldn't find a good Rally run at all when I just checked.
You might be able to reproduce a change for this kind of thing in some Rally runs that are heavy in runtime fields, those parse heavily but whether that avoids the dot expander noise is a different topic.

[smalyshev]

Maybe we could add parser-specific microbenchmark if the existing ones aren't good enough? It's not hard for me to unroll the lambdas, but if we don't have the good benchmark I can't be sure any change wouldn't be problematic so there's something that needs to be improved regardless.

[smalyshev]

@original-brownbear I've inlined the lambdas and added JSON parser microbenchmark - please see if it looks good.

[original-brownbear]

Thanks @smalyshev this looks quite nice. I could see this being a performance improvement as well actually from now reducing the code size in the hot path! Maybe wait for @rjernst to be good with this one as well but LGTM from my end!

[rjernst]

Looks much better, but I have one more question

[rjernst]

JsonFactory.createParser can throw JsonParseException, so shouldn't this catch also use handleParseException?