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

Performance Improvements #222

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

herbrandson
Copy link

This PR includes purely performance related improvements. As such, there are no new tests because there is no new functionality. I hope that's ok. If there are some tests you would like to see added to cover these changes, please let me know what you'd like to see.

The primary changes include

  1. Removing the regexs for nameStart and nameBody
  2. Removing the switch statement and replacing with a look up table
  3. Some reordering of if/else if statements to make the more probable case come first

Also, it seems that by default trackPosition ends up being true. Turing it off gives another ~10% boost. However it wasn't completely clear to me if this default could safely be changed. If the default can't be changed maybe it could at least be mentioned in the docs/examples?

Please see the attached files for a simple test case. In my testing this PR improved performance by ~%33. Unfortunately I primarily tested against only the one xml file included. Due to some time constraints on my end I didn't create a very broad test suite :(. If you'd like to see the performance proven against a wider array of example files, let me know and I'll try to put something together.

tests.zip

@@ -997,513 +1048,45 @@
}
}

switch (parser.state) {
Copy link
Author

Choose a reason for hiding this comment

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

Each case in this switch has been moved into its own function.

@@ -962,6 +980,42 @@
return result
}

var parsers = {}
Copy link
Author

Choose a reason for hiding this comment

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

This creates a mapping from each state to the correct function. Because of the number of branches in the original switch statement, this approach leads to performance gains due to significantly fewer compares/jumps.

@@ -1562,4 +1145,460 @@
}
}())
}

Copy link
Author

Choose a reason for hiding this comment

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

This is the beginning of the functions that were extracted from the original switch statement. For the most part they are just copied/pasted from their original location w/ little to no alteration (other then changing continue to return in a few places)

@herbrandson
Copy link
Author

@isaacs I see there are a few PR's on this repo that date back a ways. Is this project still actively maintained?

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.

1 participant