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 parsing performance #135

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

intsuc
Copy link

@intsuc intsuc commented Aug 11, 2023

Overview

This optimization avoids unnecessary object allocations by not taking snapshots of the parsing state when the size of the relevant nodes is 1. Parsing time becomes about 48.6 to 57.1% of the original.

Benchmark Results

Before Optimization f20bede

Benchmark Mode Cnt Score Error Units
ParsingBenchmarks.parse_ avgt 25 102.391 ± 1.121 ns/op
ParsingBenchmarks.parse_a1i avgt 25 322.832 ± 3.665 ns/op
ParsingBenchmarks.parse_c avgt 25 102.086 ± 1.063 ns/op
ParsingBenchmarks.parse_k1i avgt 25 338.494 ± 2.231 ns/op
ParsingBenchmarks.parse_l123456789 avgt 25 1014.364 ± 9.041 ns/op

After Optimization f4d27ab

Benchmark Mode Cnt Score Error Units
ParsingBenchmarks.parse_ avgt 25 52.451 ± 0.291 ns/op
ParsingBenchmarks.parse_a1i avgt 25 164.723 ± 1.224 ns/op
ParsingBenchmarks.parse_c avgt 25 52.949 ± 0.291 ns/op
ParsingBenchmarks.parse_k1i avgt 25 193.371 ± 1.875 ns/op
ParsingBenchmarks.parse_l123456789 avgt 25 492.890 ± 5.163 ns/op

This optimization avoids unnecessary object allocations by not taking snapshots of the parsing state when the relevant nodes are unique.
According to JMH's JMHSamples_08_DeadCode, the correct way of benchmarking is either returning the result or consuming it with Blackholes.
Comment on lines 364 to 365
final CommandContextBuilder<S> context = unique ? contextSoFar : contextSoFar.copy();
final StringReader reader = unique ? originalReader : new StringReader(originalReader);
Copy link
Author

Choose a reason for hiding this comment

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

If unique, we do not need to take snapshots of the parsing state (contextSoFar and originalReader) here.

Comment on lines +396 to +398
if (unique) {
return parse;
}
Copy link
Author

Choose a reason for hiding this comment

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

If unique, the potentials will always contain only parse and this parse will be returned after exiting this for loop, so we can return parse here without allocating potentials.

if (potentials == null) {
potentials = new ArrayList<>(1);
potentials = new ArrayList<>(relevantNodes.size());
Copy link
Author

Choose a reason for hiding this comment

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

Since potentials will contain ParseResults<S> up to the amount of relevantNodes.size(), allocate that capacity in advance to ensure resizing never occurs.

Copy link

@yanisderbikov yanisderbikov left a comment

Choose a reason for hiding this comment

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

I think this is less readable. But if it increase performance it's ok.

Choose a reason for hiding this comment

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

I think this is less readable. But if it increase performance it's ok.

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