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

export errors and reduce box usage #67

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

xMAC94x
Copy link
Contributor

@xMAC94x xMAC94x commented Jul 1, 2024

Hey :)

I did another PR focusing on execution speed and Errors.

  • extended the JsonPathParserError error crate with more custom things to get rid of the plain-String errors. This way its easier for libraries to handle errors and catch them.
  • switch the boolean parsing to a own function rather than invoking another json parse.
  • put the pest error in a box (because its 270 bytes long, which means that the Result is very large, even in the good case, because we always need to reserve those bytes to be prepared for the bad case)
  • The function json_path_instance returned a Box<dyn> which means that the type is determined on runtime, and this is compiled via a vtable. As all posibilities are known i just created a simple enum containing those and implementing the Path trait. This allows the compiler to further optimize the crate.

There are a few drawbacks of this change we should consider:

  • I hope i got the Error messages correct, I dont know the parser too well, but i hope they make sense
  • We no longer export the Pairs in Error directly, because the lifetimes made problems with TryFrom trait, so for now its just the String representation of the Pairs
  • I hope the custom str_to_bool function can never fail, IMO the path is only Path::boolean if the str is either true or false as described in the .pest file, but I have never worked with that parse, is this assumption true?
  • to avoid String generation in parse_logic_or and parse_logic_and functions, i did a check on the len of that iterator. IMO as soon as there is 1 entry, the result cannot be None is this correct ? Otherwise I would have introduced a logical bug, but this way we only prepare the Error message on demand`.

So this change needs some good code-review before we just merge it, i don't want to introduce some logic bugs.

On the other hand, the compilation time for a JsonPathInst is quite a bit faster now, from over 70 us to under 15us.

I was also thinking about JsonPath and JsonPathInst, maybe its possible to just use the JsonPath in the future, and also move the 3 find methods into this struct. This would definitly be something for another PR, but tell me what you think about it ? Because this PR contains some breaking changes, so we should prob wait with a new release for a bit after merging this.

@besok
Copy link
Owner

besok commented Jul 2, 2024

Hi!
Thank you for the PR. The improvements are excellent.

I will take a look at it in the evening

Copy link
Owner

@besok besok left a comment

Choose a reason for hiding this comment

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

looks good to me

@besok
Copy link
Owner

besok commented Jul 2, 2024

I hope i got the Error messages correct, I dont know the parser too well, but i hope they make sense

Yeah, everything seems to be correct.

We no longer export the Pairs in Error directly, because the lifetimes made problems with TryFrom trait, so for now its just the String representation of the Pairs

That is fine

I hope the custom str_to_bool function can never fail, IMO the path is only Path::boolean if the str is either true or false as described in the .pest file, but I have never worked with that parse, is this assumption true?

yeah, that is correct, we can so to say guarantee that it will work.

to avoid String generation in parse_logic_or and parse_logic_and functions, i did a check on the len of that iterator. IMO as soon as there is 1 entry, the result cannot be None is this correct ? Otherwise I would have introduced a logical bug, but this way we only prepare the Error message on demand`.

yeah, that is correct. At least one atom should be presented.

On the other hand, the compilation time for a JsonPathInst is quite a bit faster now, from over 70 us to under 15us.

That is very impressive

I was also thinking about JsonPath and JsonPathInst, maybe its possible to just use the JsonPath in the future, and also move the 3 find methods into this struct. This would definitly be something for another PR, but tell me what you think about it ? Because this PR contains some breaking changes, so we should prob wait with a new release for a bit after merging this.

Fair enough.

Great job!

@besok besok merged commit 18eedb7 into besok:main Jul 2, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants