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

Improvements #28

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

Improvements #28

wants to merge 9 commits into from

Conversation

stevemao
Copy link
Contributor

add some (hopefully) useful comments
don't use regex when possible
combine some if statements
remove unnecessary isTop check

this PR increases about 15 op/s
there is no logic changes

stevemao added 3 commits July 21, 2016 21:58
add some (hopefully) useful comments
don't use regex when possible
combine some if statements
remove unnecessary `isTop` check
admittedly the performance improvement is not very obvious.
perf improves a little bit
@stevemao
Copy link
Contributor Author

stevemao commented Jul 21, 2016

@juliangruber can you enable travis for this module too? 😄
Hmm... looks like its not building on PRs.

@juliangruber
Copy link
Owner

(idk why it was disabled but it's enabled again)

@stevemao
Copy link
Contributor Author

@isaacs seems good to you?

@juliangruber
Copy link
Owner

i'm still in the process of reviewing the "general improvements" commit, just want to make sure this doesn't change anything that isn't covered by tests.

this module is being used a lot, so improving it for 15 ops/s maybe isn't the best idea, but I don't want to turn down a good effort either so let's keep evaluating.

@stevemao
Copy link
Contributor Author

stevemao commented Jul 21, 2016

No problem. I'm sure there are other places to improve in the code but I'd
like to start from here

The main purpose of this is not too much of perf improvements. More just refactoring.

On 22 Jul 2016 7:12 AM, "Julian Gruber" [email protected] wrote:

i'm still in the process of reviewing the "general improvements" commit,
just want to make sure this doesn't change anything that isn't covered by
tests.

this module is being used a lot, so improving it for 15 ops/s maybe
isn't the best idea, but I don't want to turn down a good effort either
so let's keep evaluating.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGBiLnj3fCX43xCNAkVoWfqzWhMWuAIdks5qX-C5gaJpZM4JRwXT
.

@stevemao
Copy link
Contributor Author

Using the cache and O(log(n)) algorithm for repeating 0 increases the performance quite good! On my machine, it increases from 460 op/s to 530 op/s (numbers are approximate).

@myvyang myvyang mentioned this pull request Mar 2, 2017
@myvyang myvyang mentioned this pull request Apr 7, 2017
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