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

Optimize StringBuilder based loops #50

Closed
wants to merge 1 commit into from
Closed

Optimize StringBuilder based loops #50

wants to merge 1 commit into from

Conversation

slandelle
Copy link
Collaborator

Motivation:

Nodes' internalToString and ParseExceptions uses StringBuilder based loops. Even tough those shouldn't be on the hot path if Expressions are cached, they still can be optimized.

Modifications:

  • Stop using Iterators with nested hasNext tests. Use simple for loops that have a chance of being JIT'ed with escape analysis.
  • Don't test last element in loop (cf above). Update StringBuilder's length after loop if it wasn't empty.
  • Don't use StringBuilder#delete that causes a copy of the underlying char array. Update StringBuilder's length instead as we always want to remove the tail.

Result:

Less allocations

Motivation:

Nodes' internalToString and ParseExceptions uses StringBuilder based loops. Even tough those shouldn't be on the hot path if Expressions are cached, they still can be optimized.

Modifications:

* Stop using Iterators with nested hasNext tests. Use simple for loops that have a chance of being JIT'ed with escape analysis.
* Don't test last element in loop (cf above). Update StringBuilder's length after loop if it wasn't empty.
* Don't use StringBuilder#delete that causes a copy of the underlying char array. Update StringBuilder's length instead as we always want to remove the tail.

Result:

Less allocations
Copy link
Collaborator

@iconara iconara left a comment

Choose a reason for hiding this comment

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

I don't mind the change, and since you've written the code there's no reason not to merge it, but as you say, this is not a hot path…

Please use the Request Review feature in the future, it works better to get my attention than @-ing.

@slandelle slandelle closed this Jun 7, 2019
@slandelle slandelle deleted the stringbuilder branch June 7, 2019 08:25
@slandelle
Copy link
Collaborator Author

Shoot, I deleted wrong branch. Will submit again.

@slandelle
Copy link
Collaborator Author

@iconara I've submitted again, see #52

Please use the Request Review feature in the future, it works better to get my attention than @-ing.

I'd love to, but Request Review feature requires collaborator permission.

@iconara
Copy link
Collaborator

iconara commented Jun 7, 2019

I'd love to, but Request Review feature requires collaborator permission.

Ah, I had no idea. That restriction makes no sense.

@iconara
Copy link
Collaborator

iconara commented Jun 7, 2019

I've added you as a "triage" collaborator.

@slandelle
Copy link
Collaborator Author

Awesome, thanks!

@iconara
Copy link
Collaborator

iconara commented Jun 7, 2019

There's also a new snapshot release with all your PRs: https://github.com/burtcorp/jmespath-java/releases/tag/jmespath-0.3.1-SNAPSHOT

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.

None yet

2 participants