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

reduce buffer creation for ctr mode #48

Merged
merged 1 commit into from Aug 18, 2017

Conversation

Projects
None yet
5 participants
@dignifiedquire
Contributor

dignifiedquire commented Aug 16, 2017

When using ctr mode, I saw a large amount of buffer creation + gc when running perf analysis. This avoids generating most of the temporary buffers when running update in ctr mode improving perf in the browser nicely.

self._cache.writeUInt32BE(out[0], offset + 0)
self._cache.writeUInt32BE(out[1], offset + 4)
self._cache.writeUInt32BE(out[2], offset + 8)
self._cache.writeUInt32BE(out[3], offset + 12)

This comment has been minimized.

@dcousens

dcousens Aug 17, 2017

Member

Is this really better than a copy?

@dcousens

dcousens Aug 17, 2017

Member

Is this really better than a copy?

This comment has been minimized.

@dignifiedquire

dignifiedquire Aug 17, 2017

Contributor

The writeUint is not better, but the reason I am doing this is that it avoids creating intermediary buffers. Before encryptBlock creates a new buffer, fills it with those four values, just so that the buffer can be copied in here. By using the array directly there is now one less buffer allocation in the loop and together with the preallocation, no buffer allocations are in the loop.

@dignifiedquire

dignifiedquire Aug 17, 2017

Contributor

The writeUint is not better, but the reason I am doing this is that it avoids creating intermediary buffers. Before encryptBlock creates a new buffer, fills it with those four values, just so that the buffer can be copied in here. By using the array directly there is now one less buffer allocation in the loop and together with the preallocation, no buffer allocations are in the loop.

This comment has been minimized.

@dcousens

dcousens Aug 17, 2017

Member

Nevermind, my mistake, I thought out was a Buffer.
ACK :)

@dcousens

dcousens Aug 17, 2017

Member

Nevermind, my mistake, I thought out was a Buffer.
ACK :)

@diasdavid

👌🏽

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Aug 17, 2017

Member
Member

calvinmetcalf commented Aug 17, 2017

@dignifiedquire

This comment has been minimized.

Show comment
Hide comment
@dignifiedquire

dignifiedquire Aug 17, 2017

Contributor

@calvinmetcalf that's just githubs diff I pulled part of the function into it's own function, as I needed that functionality

Contributor

dignifiedquire commented Aug 17, 2017

@calvinmetcalf that's just githubs diff I pulled part of the function into it's own function, as I needed that functionality

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Aug 17, 2017

Member

oh my bad

Member

calvinmetcalf commented Aug 17, 2017

oh my bad

@dcousens dcousens merged commit 040d953 into crypto-browserify:master Aug 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Aug 18, 2017

Member

@calvinmetcalf could you release?

Member

dcousens commented Aug 18, 2017

@calvinmetcalf could you release?

@ya7ya

This comment has been minimized.

Show comment
Hide comment
@ya7ya

ya7ya Sep 3, 2017

hey @calvinmetcalf @dcousens , When do you think this can be released ?

ya7ya commented Sep 3, 2017

hey @calvinmetcalf @dcousens , When do you think this can be released ?

@dignifiedquire dignifiedquire deleted the dignifiedquire:faster-ctr branch Sep 3, 2017

@diasdavid

This comment has been minimized.

Show comment
Hide comment
@diasdavid

diasdavid commented Sep 3, 2017

@calvinmetcalf @dcousens yes please! :)

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Sep 4, 2017

Member

Can't release yet, my local tests throw:

TypeError: invalid key length 128

Investingating within 24 hours

Member

dcousens commented Sep 4, 2017

Can't release yet, my local tests throw:

TypeError: invalid key length 128

Investingating within 24 hours

@diasdavid diasdavid referenced this pull request Sep 11, 2017

Closed

⚡️ v0.26.0 RELEASE 🚀 #986

16 of 16 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment