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 #9

Merged
merged 4 commits into from
Jan 10, 2015
Merged

Conversation

arthurschreiber
Copy link
Collaborator

This is a list of performance improvements I discovered while implementing tediousjs/tedious#225.

@deoxxa Are you still maintining this library?

`buffer` and `string` jobs can specify their lengths using the name of
another, previously parsed value. This is handled by copying the job
definition and replacing the job's length property.

Instead of copying the job, it's much better to just store the length
in a variable instead. This slightly improves the performance of these
job types and reduces the number of objects being created, thus improving
GC times.
Instead of slicing and splicing the jobs array at the start of a `loop`
or `tap` job, we can stash away a reference to the current jobs list and
just replace it with an empty array.

Then, after the respective job has finished, we can push back the stashed
jobs into this new job list array.

This improves performance (because slice and splice are costly operations)
and reduces the number of created objects (because both splice and slice
create new array objects).
Re-use the existing `slice` function that is provided by `BufferList`.
It has some internal optimizations which make it faster than just copying
from the BufferList object into a newly allocated buffer.
This makes the code a bit easier to profile, and allows V8 to be
more aggressive about optimizations.
deoxxa added a commit that referenced this pull request Jan 10, 2015
@deoxxa deoxxa merged commit edbff6d into deoxxa:master Jan 10, 2015
@deoxxa
Copy link
Owner

deoxxa commented Jan 10, 2015

Hello! I'm afraid I haven't used this library much in a while, and I probably won't use it much in the future. I'm going to add you as a maintainer and an owner on NPM, so you can make changes and releases as you see fit. Feel free also to add anybody else you see as suitable for maintaining this code.

@deoxxa
Copy link
Owner

deoxxa commented Jan 10, 2015

Or at least, I will add you on npm, as soon as I know your username! haha

@arthurschreiber
Copy link
Collaborator Author

@deoxxa Thanks man! My npm username is also arthurschreiber.

@arthurschreiber arthurschreiber deleted the arthur/perf branch January 10, 2015 12:33
@deoxxa
Copy link
Owner

deoxxa commented Jan 10, 2015

All done! Are you by any chance using concentrate as well?

@arthurschreiber
Copy link
Collaborator Author

Not yet. But if you're looking for some help in maintaining it, I'd be up for the task.

@deoxxa
Copy link
Owner

deoxxa commented Jan 10, 2015

I've already foisted it onto @marushkevych in a way, but I don't expect there'd be much to do on it. I've just always seen the two libraries as two halves of a complete system, so I think it'd make sense to have people with access to one have access to the other as well. I'll add you on it, but you're not under any obligation to actually do anything unless you want or need 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