Change the QuicEncrypter to accept a pre-allocated buffer instead of

Change the QuicEncrypter to accept a pre-allocated buffer instead of
allocating it's own buffer.  No functional change.

Merge internal change: 85952670

R=rch@chromium.org

Committed: https://crrev.com/eb50361cba0ab7d2ff0ebd400673bbae2afd465f
Cr-Commit-Position: refs/heads/master@{#316320}

[Ryan Hamilton, 2015-02-11 00:35:54]

lgtm

[ramant (doing other things), 2015-02-11 19:45:06]

New patchsets have been uploaded after l-g-t-m from rch@chromium.org

[ramant (doing other things), 2015-02-11 20:21:51]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2015-02-11 20:22:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/912163002/20001

[ramant (doing other things), 2015-02-11 20:42:47]

On 2015/02/11 20:22:25, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/912163002/20001

Hi Misha,
  This is the last CL (I know of) that improves the performance (memory usage)
  of Quic Crypto code. We would appreciate changes until the CL could be
included
  in the next release of cronet (this change will go to Dev next week).

thanks
raman

[Ian Swett, 2015-02-11 21:02:15]

On 2015/02/11 20:42:47, ramant wrote:
> On 2015/02/11 20:22:25, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/912163002/20001
> 
> Hi Misha,
>   This is the last CL (I know of) that improves the performance (memory usage)
>   of Quic Crypto code. We would appreciate changes until the CL could be
> included
>   in the next release of cronet (this change will go to Dev next week).
> 
> thanks
> raman

Actually, there's one more CL that I just landed today.  Not necessary, but one
less allocation on the write side.

[mef, 2015-02-11 21:26:21]

On 2015/02/11 21:02:15, Ian Swett wrote:
> On 2015/02/11 20:42:47, ramant wrote:
> > On 2015/02/11 20:22:25, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/912163002/20001
> > 
> > Hi Misha,
> >   This is the last CL (I know of) that improves the performance (memory
usage)
> >   of Quic Crypto code. We would appreciate changes until the CL could be
> > included
> >   in the next release of cronet (this change will go to Dev next week).
> > 
> > thanks
> > raman
> 
> Actually, there's one more CL that I just landed today.  Not necessary, but
one
> less allocation on the write side.

Sounds good, let me know when everything is in and I'll update google3 with
latest build.

[commit-bot: I haz the power, 2015-02-11 21:44:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-11 21:44:45]

Try jobs failed on following builders:
  android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)

[ramant (doing other things), 2015-02-11 23:48:42]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2015-02-11 23:49:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/912163002/40001

[ramant (doing other things), 2015-02-12 01:03:56]

The CQ bit was unchecked by rtenneti@chromium.org

[ramant (doing other things), 2015-02-12 01:04:32]

On 2015/02/11 23:49:29, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/912163002/40001

This CL is crashing quic tests on Android only. Will add more debug lines to
debug it.

[ramant (doing other things), 2015-02-13 00:30:38]

Patchset #4 (id:60001) has been deleted

[ramant (doing other things), 2015-02-13 00:30:53]

Patchset #4 (id:80001) has been deleted

[ramant (doing other things), 2015-02-13 00:31:10]

Patchset #4 (id:100001) has been deleted

[ramant (doing other things), 2015-02-13 00:31:18]

Patchset #4 (id:120001) has been deleted

[ramant (doing other things), 2015-02-13 00:31:41]

Patchset #2 (id:20001) has been deleted

[ramant (doing other things), 2015-02-13 02:23:52]

Patchset #3 (id:140001) has been deleted

[ramant (doing other things), 2015-02-13 04:05:32]

Patchset #3 (id:160001) has been deleted

[ramant (doing other things), 2015-02-13 17:37:05]

Patchset #4 (id:200001) has been deleted

[ramant (doing other things), 2015-02-13 18:44:08]

Patchset #3 (id:180001) has been deleted

[davidben, 2015-02-13 20:39:35]

The crash is because the ChaCha nonce had to be 4-byte-aligned on ARM. That
requirement was silly, so we've fixed it. I'll roll it into Chromium shortly.

https://boringssl-review.googlesource.com/#/c/3460/

The immediate cause was that you're aliasing the nonce with the output which is
kind of weird, though it seems to work right now. I guess the stack-allocated
one ended up 4-byte-aligned. (By the way, we define an EVP_AEAD_MAX_NONCE_LENGTH
constant you can use for the nonce size. It's only 8 bytes for ChaCha and 16
bytes max overall.)

[Ryan Hamilton, 2015-02-13 20:48:09]

Holy cow. Wow, thanks for getting to the bottom of this!

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ramant (doing other things), 2015-02-13 22:04:56]

On 2015/02/13 20:39:35, David Benjamin wrote:
> The crash is because the ChaCha nonce had to be 4-byte-aligned on ARM. That
> requirement was silly, so we've fixed it. I'll roll it into Chromium shortly.
> 
> https://boringssl-review.googlesource.com/#/c/3460/
> 
> The immediate cause was that you're aliasing the nonce with the output which
is
> kind of weird, though it seems to work right now. I guess the stack-allocated
> one ended up 4-byte-aligned. (By the way, we define an
EVP_AEAD_MAX_NONCE_LENGTH
> constant you can use for the nonce size. It's only 8 bytes for ChaCha and 16
> bytes max overall.)

Many many thanks David for the fix.

-raman

[ramant (doing other things), 2015-02-13 22:18:29]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2015-02-13 22:18:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/912163002/240001

[commit-bot: I haz the power, 2015-02-13 23:23:31]

Committed patchset #4 (id:240001)

[commit-bot: I haz the power, 2015-02-13 23:24:12]

Patchset 4 (id:??) landed as
https://crrev.com/eb50361cba0ab7d2ff0ebd400673bbae2afd465f
Cr-Commit-Position: refs/heads/master@{#316320}