Skip to content

Conversation

@abdulhannanali
Copy link

@abdulhannanali abdulhannanali commented Nov 24, 2017

Small pull requests which makes minor performance improvements to the libraries

  • Eliminates an additional tick by checking the length before hand and if it needs to be started (fyi each unnecessary tick hurts a kitten 🐱 )
  • Use filter instead of splice
    Removing the use of splice and for loop in destroy method with filter since it's more efficient and IMO more clearer though.

Additional discussions:

  • Replacing the for loop in tick function too
    I did not make a PR with that, since that's a big function and there's only one scenario (when animation ends) we need to make any mutations to the array, so doing a filter will make the code clunky, but this is something I would like to discuss further.

have a nice day. thanks for this library

@pakastin
Copy link
Owner

Actually I think custom for loop filter would be even faster.. Good changes, I'll check out these in the evening – thank you!!

@pakastin
Copy link
Owner

I haven't been updating this for a while, there's a whole lot more changes I'd make today so I check those out first.. 😉

@abdulhannanali
Copy link
Author

@pakastin Yup custom for loop filter would be faster, and we can do a remove function, which can be used at both places but the problem with that is, more code, while filter is simple and fast enough for our usage, there's a considerable difference between splice and the filter method, custom for loop filter overkill maybe, as it's going to be used at only one place. Check out the jsPerf benchmarks I linked to 😄

1 similar comment
@abdulhannanali
Copy link
Author

@pakastin Yup custom for loop filter would be faster, and we can do a remove function, which can be used at both places but the problem with that is, more code, while filter is simple and fast enough for our usage, there's a considerable difference between splice and the filter method, custom for loop filter overkill maybe, as it's going to be used at only one place. Check out the jsPerf benchmarks I linked to 😄

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