Simplify o.e.i.m.ContentPath #94314
Simplify o.e.i.m.ContentPath #94314
Conversation
Just some random finds from looking into this code. The index in the content path is always zero these days so we can just simplify it away. The indirection to the path's leaf object flag actually has overhead since we had to paths creating different lambda's so it became a bimorphic callsite, removing that indirection which was only used in tests anyway makes the code easier to read also in my opinion. Lastly, we were creating a needless empty string instance in the validation of end-of-document logic.
original-brownbear
added
>non-issue
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
cbuescher
left a comment
cbuescher
left a comment
There was a problem hiding this comment.
Thanks a lot, +1 on the better readability. I left one small ask that's hopefully not too complicated to address.
| if (remainingPath.isEmpty() == false) { | ||
| throwOnLeftoverPathElements(remainingPath); | ||
| if (context.path.atRoot() == false) { | ||
| throwOnLeftoverPathElements(context.path); |
There was a problem hiding this comment.
Since the exception is only thrown here, can we inline it for readability?
Also the case where this exception is thrown seems untested, would it be possible to add a check to DocumentParserTests?
There was a problem hiding this comment.
Hmm I think it might be impossible to get to this code?
I can only see getting here via broken JSON, but the JSON (or other type) parsers will throw on malformed input.
So it seems impossible to ever get through parsing any document without the appropriate number of closing brackets?
I the exception is thrown in a separate method as a bit of a questionable JIT optimization. But now that I think about the above ... can't we just remove this code?
There was a problem hiding this comment.
Sure, let me take another closer look to understand your reasoning, all in favor of removing it if we don't even test it.
There was a problem hiding this comment.
I took another look, the only way I see this exception can be thrown here is if there were calls to path.add() somewhere inside the parsing logic that wouldn't be correctly followed by a path.remove() operation. While that is certainly possible due to a programming error, that sounds much better suited to be a test or an assertion at this point than an exception.
I would opt for make this an assertion to communicate the invariant we have here and potentially catch errors in tests where we have assertions enabled.
There was a problem hiding this comment.
++ assertion makes, sense pushed 15e0f32
|
Jenkins run elasticsearch-ci/part-3 (unrelated + known issue) |
3 participants
Just some random finds from looking into this code. The index in the content path is always zero these days so we can just simplify it away. The indirection to the path's leaf object flag actually has overhead since we had to paths creating different lambda's so it became a bimorphic callsite, removing that indirection which was only used in tests anyway makes the code easier to read also in my opinion. Lastly, we were creating a needless empty string instance in the validation of end-of-document logic.