Restore SSL_SESSION/X509Certificate X509* sharing

Restore SSL_SESSION/X509Certificate X509* sharing

This effectively reverts https://codereview.chromium.org/2300533002
and replaces it with a smarter X.509 representation. We haven't
gotten rid of X509* completely yet, but we're far enough along there
that we can improve this.

There are three flavors of X509* that Chromium potentially keeps
around in memory:

1. Vanilla X509*. This does not make the full DER form easily
   accessible, but does cache the DER form of the
   TBSCertificate.

2. X509* + DERCache. This is (1) with an extra copy of the
   full DER form accessible. The DER form is accessible (needed
   by lots of things) but we use a bunch more memory.

3. X509* + CRYPTO_BUFFER. This is a smarter version of (2). The
   full DER form is stored in a CRYPTO_BUFFER but rather than
   have it waste memory, we alias the cached TBSCertificate into
   it.

(3) was added early on in switching from X509 to CRYPTO_BUFFER.
When https://codereview.chromium.org/2300533002 was done, (3) did
not exist, so we had this split where X509s hanging off
SSL_SESSION did not need DERCache but net::X509Certificate did.
Sharing the X509* was a nice memory optimization in one direction
(fewer X509s in memory---X509s are really *really* inefficient,
which is one of the motivations in removing them), but at the
cost of stapling DERCache to more things.

Whenever the SSL_SESSION's reference outlives the
net::X509Certificate's reference, 2300533002 is a win. When both
are alive in memory, it is a loss.

Now, every SSL_SESSION-owned X509 is of type (3) anyway, so we can
be smarter. Undo the X509*-side regression and instead just don't
staple DERCache onto X509*s of type (3). They already have a free
DERCache on them. This achieves 2300533002's goals without the
X509* tradeoff.

BUG=671420, 642082
Review-Url: https://codereview.chromium.org/2694903006
Cr-Commit-Position: refs/heads/master@{#453455}
Committed: https://chromium.googlesource.com/chromium/src/+/50fee4d87b534d63aa9769d1c593c368a320edcd

[davidben, 2017-02-15 21:30:14]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 21:31:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2017-02-15 21:43:01]

The CQ bit was checked by davidben@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 21:43:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 23:28:13]

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

[commit-bot: I haz the power, 2017-02-15 23:28:14]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[davidben, 2017-02-17 23:23:49]

Description was changed from

==========
Experimental

TODO write description

BUG=671420
==========

to

==========
Restore SSL_SESSION/X509Certificate X509* sharing

This effectively reverts https://codereview.chromium.org/2300533002
and replaces it with a smarter X.509 representation. We haven't
gotten rid of X509* completely yet, but we're far enough along there
that we can improve this.

There are three flavors of X509* that Chromium potentially keeps
around in memory:

1. Vanilla X509*. This does not make the full DER form easily
   accessible, but does cached the DER form of the
   TBSCertificate.

2. X509* + DERCache. This is (1) with an extra copy of the
   full DER form accessible. The DER form is accessible (needed
   by lots of things) but we use a bunch more memory.

3. X509* + CRYPTO_BUFFER. This is a smarter version of (2). The
   full DER form is stored in a CRYPTO_BUFFER but rather than
   have it waste memory, we alias the cached TBSCertificate into
   it.

(3) was added early on in switching from X509 to CRYPTO_BUFFER.
When https://codereview.chromium.org/2300533002 was done, (3) did
not exist, so we had this split where X509s hanging off
SSL_SESSION did not need DERCache but net::X509Certificate did.
Sharing the X509* was a nice memory optimization in one direction
(fewer X509s in memory---X509s are really *really* inefficient,
which is one of the motivations in removing them), but at the
cost of stapling DERCache to more things.

If the SSL_SESSION's reference outlived the net::X509Certificate's
reference, the change was a win. While both are alive in memory,
the change is a loss.

Now, every SSL_SESSION-owned X509 is of type (3) anyway, so we can
be smarter. Undo the X509*-side regression and instead just don't
staple DERCache onto X509*s of type (3). They already have a free
DERCache on them.

BUG=671420,642082
==========

[davidben, 2017-02-17 23:24:23]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2017-02-18 00:05:19]

lgtm

[xunjieli, 2017-02-21 15:54:05]

drive-by. Thanks for providing the detailed context in the cl description.

Three nits below:

> This does not make the full DER form easily accessible, but does cached the
DER form of the TBSCertificate.

nit: s/cached/cache

> If the SSL_SESSION's reference outlived the net::X509Certificate's reference,
the change was a win. While both are alive in memory, the change is a loss.

nit: Make it clearer as to which "change" this sentence refers to? It sounds
like the current changelist on a first read. Could put the cl number in a
bracket after the first word "change," or merge this paragraph with the previous
one.

https://codereview.chromium.org/2694903006/diff/20001/net/cert/x509_util_open...
File net/cert/x509_util_openssl.cc (right):

https://codereview.chromium.org/2694903006/diff/20001/net/cert/x509_util_open...
net/cert/x509_util_openssl.cc:274: // (note: the DER-encoded data in |der_cache|
is owned by |cert|, callers should
nit: maybe update the comment. "|cert|" isn't present in the code.

[davidben, 2017-02-28 00:16:29]

Description was changed from

==========
Restore SSL_SESSION/X509Certificate X509* sharing

This effectively reverts https://codereview.chromium.org/2300533002
and replaces it with a smarter X.509 representation. We haven't
gotten rid of X509* completely yet, but we're far enough along there
that we can improve this.

There are three flavors of X509* that Chromium potentially keeps
around in memory:

1. Vanilla X509*. This does not make the full DER form easily
   accessible, but does cached the DER form of the
   TBSCertificate.

2. X509* + DERCache. This is (1) with an extra copy of the
   full DER form accessible. The DER form is accessible (needed
   by lots of things) but we use a bunch more memory.

3. X509* + CRYPTO_BUFFER. This is a smarter version of (2). The
   full DER form is stored in a CRYPTO_BUFFER but rather than
   have it waste memory, we alias the cached TBSCertificate into
   it.

(3) was added early on in switching from X509 to CRYPTO_BUFFER.
When https://codereview.chromium.org/2300533002 was done, (3) did
not exist, so we had this split where X509s hanging off
SSL_SESSION did not need DERCache but net::X509Certificate did.
Sharing the X509* was a nice memory optimization in one direction
(fewer X509s in memory---X509s are really *really* inefficient,
which is one of the motivations in removing them), but at the
cost of stapling DERCache to more things.

If the SSL_SESSION's reference outlived the net::X509Certificate's
reference, the change was a win. While both are alive in memory,
the change is a loss.

Now, every SSL_SESSION-owned X509 is of type (3) anyway, so we can
be smarter. Undo the X509*-side regression and instead just don't
staple DERCache onto X509*s of type (3). They already have a free
DERCache on them.

BUG=671420,642082
==========

to

==========
Restore SSL_SESSION/X509Certificate X509* sharing

This effectively reverts https://codereview.chromium.org/2300533002
and replaces it with a smarter X.509 representation. We haven't
gotten rid of X509* completely yet, but we're far enough along there
that we can improve this.

There are three flavors of X509* that Chromium potentially keeps
around in memory:

1. Vanilla X509*. This does not make the full DER form easily
   accessible, but does cache the DER form of the
   TBSCertificate.

2. X509* + DERCache. This is (1) with an extra copy of the
   full DER form accessible. The DER form is accessible (needed
   by lots of things) but we use a bunch more memory.

3. X509* + CRYPTO_BUFFER. This is a smarter version of (2). The
   full DER form is stored in a CRYPTO_BUFFER but rather than
   have it waste memory, we alias the cached TBSCertificate into
   it.

(3) was added early on in switching from X509 to CRYPTO_BUFFER.
When https://codereview.chromium.org/2300533002 was done, (3) did
not exist, so we had this split where X509s hanging off
SSL_SESSION did not need DERCache but net::X509Certificate did.
Sharing the X509* was a nice memory optimization in one direction
(fewer X509s in memory---X509s are really *really* inefficient,
which is one of the motivations in removing them), but at the
cost of stapling DERCache to more things.

Whenever the SSL_SESSION's reference outlives the
net::X509Certificate's reference, 2300533002 is a win. When both
are alive in memory, it is a loss.

Now, every SSL_SESSION-owned X509 is of type (3) anyway, so we can
be smarter. Undo the X509*-side regression and instead just don't
staple DERCache onto X509*s of type (3). They already have a free
DERCache on them. This achieves 2300533002's goals without the
X509* tradeoff.

BUG=671420,642082
==========

[davidben, 2017-02-28 00:17:43]

> nit: s/cached/cache

Done.

> nit: Make it clearer as to which "change" this sentence refers to? It sounds
like the current changelist on a first read. Could put the cl number in a
bracket after the first word "change," or merge this paragraph with the previous
one.

Done.

https://codereview.chromium.org/2694903006/diff/20001/net/cert/x509_util_open...
File net/cert/x509_util_openssl.cc (right):

https://codereview.chromium.org/2694903006/diff/20001/net/cert/x509_util_open...
net/cert/x509_util_openssl.cc:274: // (note: the DER-encoded data in |der_cache|
is owned by |cert|, callers should
On 2017/02/21 15:54:05, xunjieli wrote:
> nit: maybe update the comment. "|cert|" isn't present in the code.

Done.

[davidben, 2017-02-28 00:17:58]

The CQ bit was checked by davidben@chromium.org

[davidben, 2017-02-28 00:17:58]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2694903006/#ps40001
(title: "xunjieli comments")

[commit-bot: I haz the power, 2017-02-28 00:18:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 02:13:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488241078244910,
"parent_rev": "fe586ab75aca1b8ab839db23bceac5f621389fed", "commit_rev":
"50fee4d87b534d63aa9769d1c593c368a320edcd"}

[commit-bot: I haz the power, 2017-02-28 02:13:43]

Description was changed from

==========
Restore SSL_SESSION/X509Certificate X509* sharing

This effectively reverts https://codereview.chromium.org/2300533002
and replaces it with a smarter X.509 representation. We haven't
gotten rid of X509* completely yet, but we're far enough along there
that we can improve this.

There are three flavors of X509* that Chromium potentially keeps
around in memory:

1. Vanilla X509*. This does not make the full DER form easily
   accessible, but does cache the DER form of the
   TBSCertificate.

2. X509* + DERCache. This is (1) with an extra copy of the
   full DER form accessible. The DER form is accessible (needed
   by lots of things) but we use a bunch more memory.

3. X509* + CRYPTO_BUFFER. This is a smarter version of (2). The
   full DER form is stored in a CRYPTO_BUFFER but rather than
   have it waste memory, we alias the cached TBSCertificate into
   it.

(3) was added early on in switching from X509 to CRYPTO_BUFFER.
When https://codereview.chromium.org/2300533002 was done, (3) did
not exist, so we had this split where X509s hanging off
SSL_SESSION did not need DERCache but net::X509Certificate did.
Sharing the X509* was a nice memory optimization in one direction
(fewer X509s in memory---X509s are really *really* inefficient,
which is one of the motivations in removing them), but at the
cost of stapling DERCache to more things.

Whenever the SSL_SESSION's reference outlives the
net::X509Certificate's reference, 2300533002 is a win. When both
are alive in memory, it is a loss.

Now, every SSL_SESSION-owned X509 is of type (3) anyway, so we can
be smarter. Undo the X509*-side regression and instead just don't
staple DERCache onto X509*s of type (3). They already have a free
DERCache on them. This achieves 2300533002's goals without the
X509* tradeoff.

BUG=671420,642082
==========

to

==========
Restore SSL_SESSION/X509Certificate X509* sharing

This effectively reverts https://codereview.chromium.org/2300533002
and replaces it with a smarter X.509 representation. We haven't
gotten rid of X509* completely yet, but we're far enough along there
that we can improve this.

There are three flavors of X509* that Chromium potentially keeps
around in memory:

1. Vanilla X509*. This does not make the full DER form easily
   accessible, but does cache the DER form of the
   TBSCertificate.

2. X509* + DERCache. This is (1) with an extra copy of the
   full DER form accessible. The DER form is accessible (needed
   by lots of things) but we use a bunch more memory.

3. X509* + CRYPTO_BUFFER. This is a smarter version of (2). The
   full DER form is stored in a CRYPTO_BUFFER but rather than
   have it waste memory, we alias the cached TBSCertificate into
   it.

(3) was added early on in switching from X509 to CRYPTO_BUFFER.
When https://codereview.chromium.org/2300533002 was done, (3) did
not exist, so we had this split where X509s hanging off
SSL_SESSION did not need DERCache but net::X509Certificate did.
Sharing the X509* was a nice memory optimization in one direction
(fewer X509s in memory---X509s are really *really* inefficient,
which is one of the motivations in removing them), but at the
cost of stapling DERCache to more things.

Whenever the SSL_SESSION's reference outlives the
net::X509Certificate's reference, 2300533002 is a win. When both
are alive in memory, it is a loss.

Now, every SSL_SESSION-owned X509 is of type (3) anyway, so we can
be smarter. Undo the X509*-side regression and instead just don't
staple DERCache onto X509*s of type (3). They already have a free
DERCache on them. This achieves 2300533002's goals without the
X509* tradeoff.

BUG=671420,642082

Review-Url: https://codereview.chromium.org/2694903006
Cr-Commit-Position: refs/heads/master@{#453455}
Committed:
https://chromium.googlesource.com/chromium/src/+/50fee4d87b534d63aa9769d1c593...
==========

[commit-bot: I haz the power, 2017-02-28 02:13:44]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/50fee4d87b534d63aa9769d1c593...