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

Improve XML scanner memory #1206

Merged
merged 1 commit into from
May 23, 2022

Conversation

angelozerr
Copy link
Contributor

Improve XML scanner memory

Signed-off-by: azerr azerr@redhat.com

Signed-off-by: azerr <azerr@redhat.com>
@angelozerr
Copy link
Contributor Author

angelozerr commented May 23, 2022

This PR improves the XML scanner memory by using static int fields pattern instead of creating for each parse the int array pattern.

If you run https://github.com/eclipse/lemminx/blob/master/org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/performance/XMLScannerPerformance.java you can see the memory which takes around 200MB to parse the large file:

image

If you start this XMLScannerPerformance inmaster branch, it takes around 300-400MB:

image

@angelozerr angelozerr added this to the 0.21.0 milestone May 23, 2022
@angelozerr angelozerr requested a review from fbricon May 23, 2022 14:12
@@ -126,7 +126,7 @@ public boolean advanceIfChar(int ch) {
return false;
}

public boolean advanceIfChars(int... ch) {
public boolean advanceIfChars(int[] ch) {
Copy link
Contributor

@fbricon fbricon May 23, 2022

Choose a reason for hiding this comment

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

is there a perf penalty when declaring varargs here? You could still call stream.advanceIfChars(END_COMMENT_PATTERN) with an actual array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer updating to int array to force to use of static array. If we let varargs, it means that we could not define static array for future improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok fair enough

Copy link
Contributor

@fbricon fbricon left a comment

Choose a reason for hiding this comment

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

Not tested but LGTM

@angelozerr angelozerr merged commit 65dc7dc into eclipse-lemminx:master May 23, 2022
@angelozerr
Copy link
Contributor Author

Thanks @fbricon !

@angelozerr angelozerr added the performance This issue or enhancement is related to performance concerns label Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance This issue or enhancement is related to performance concerns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants