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

Optimizations #30

Merged
merged 1 commit into from
Sep 15, 2017
Merged

Optimizations #30

merged 1 commit into from
Sep 15, 2017

Conversation

ibesora
Copy link

@ibesora ibesora commented Sep 15, 2017

Things changed

src/labelgun.js

  • Changed some for loops that were using Object.keys(this.allLabels) for a for-in loop. hasOwnProperty is not checked because allLabels is an empty object so it doesn"t inherit nothing via prototype
  • Made the following changes to the _compareLabels function:
    • Removed the Object.keys().map() with a simple for-in loop
    • Changed the order of the if conditionals to better short circuit evaluation and to just execute the _allLower function on last resort
  • Changed the compare function to a simple substraction, removing both ifs
  • Changed the setupLabels function to do the bulk insertion to the tree when possible. As seen in rbush documentation it"s ~2-3 times faster than inserting items one by one

spec/labelgun.spec.js

  • Modified some tests to take into account the need to explicitly call the update method when the insertion is finished

Results

20 executions of the benchmark clocked at an average of 25.6 milliseconds before the optimization.
After the optimization the average is 12.3 milliseconds. Twice as fast.

@ibesora ibesora mentioned this pull request Sep 15, 2017
@JamesLMilner JamesLMilner merged commit 7c54eb1 into Geovation:master Sep 15, 2017
@JamesLMilner
Copy link
Contributor

Hey @ibesora firstly thank you for your work, greatly appreciated! Generally looks good. It looks like the tests all pass. I"ll merge it!

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