Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[tls] write data by small (1500 bytes) chunks, see http://www.belshe.com/ #961

Closed
wants to merge 2 commits into from

6 participants

Fedor Indutny ry Paul Querna Nodejs Jenkins isaacs Alexey Kupershtokh
Fedor Indutny
Owner

This pull request was previously posted here, but this time it's low-level:

P.S.
See http://www.belshe.com/2010/12/17/performance-and-the-tls-record-size/

ry
ry commented

benchmark?

Fedor Indutny
Owner

Sorry, no benchmarks. That is optimization for server client's - read article carefully! (Last time I'd posted that pull request - I missed that fact)

Fedor Indutny
Owner

Basically, if you're sending large chunks - browser can display parts of page before it stop receiving, b/c part of chunk can't be encrypted.

Fedor Indutny
Owner

BTW, try this in Chrome:
https://donnerjack13589.dyndns.tv:8081/

It's nodejs SPDY server :) And it's sending data in 1500 bytes chunks

Paul Querna
Owner

I'm concerned about the effect this might have on throughput, and thats what it would be nice to have a benchmark of -- showing the effect is negligible.

1500 byte writes to SSL_write also would cause a bad behavior for many connections, because that would become 1500+ bytes with the TLS headers/protocol, and for most MTUs would end up being 2 packets instead of 1. (good thing to look at in wireshark over non-localhost)

If the benchmark shows this isn't a huge drop in throughput, I'd be interested in seeing what happens with using ~1300-1400 byte writes, which should drop you bellow the common MTUs.

Fedor Indutny
Owner

Hm... Nice point, @pquerna.

Mike has told me to use 1400 bytes, but I'd seen 1500 in chromium's flip_server - so I decided to use last one...
Probably that was wrong step.

Can you please help and perform some tests?

Fedor Indutny
Owner

I think we should come back to this issue, splitting can't be done in userland

Fedor Indutny
Owner

Reconsider?

Nodejs Jenkins

Can one of the admins verify this patch?

Fedor Indutny
Owner

fuck it.

Fedor Indutny indutny closed this
isaacs
Owner

fuck it.

@indutny you should be more friendly to pull request submitters ;P

Alexey Kupershtokh

Is it a special command for jenkins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 36 additions and 7 deletions.
  1. +2 −4 lib/tls.js
  2. +34 −3 src/node_crypto.cc
6 lib/tls.js
View
@@ -382,8 +382,8 @@ CryptoStream.prototype._pull = function() {
this.pair._maybeInitFinished();
- if (rv === 0 || rv < 0) {
- this._pending.unshift(tmp);
+ if (rv < tmp.length) {
+ this._pending.unshift(tmp.slice(Math.max(rv, 0)));
this._pendingCallbacks.unshift(cb);
break;
}
@@ -392,8 +392,6 @@ CryptoStream.prototype._pull = function() {
assert(this._pendingBytes >= 0);
if (cb) cb();
-
- assert(rv === tmp.length);
}
// If we've cleared all of incoming encrypted data, emit drain.
37 src/node_crypto.cc
View
@@ -959,12 +959,43 @@ Handle<Value> Connection::ClearIn(const Arguments& args) {
if (rv < 0) return scope.Close(Integer::New(rv));
}
- int bytes_written = SSL_write(ss->ssl_, buffer_data + off, len);
+ const int chunkSize = 1500;
+ int rv = 0;
+ int offset = 0;
+ int total_bytes_written = 0;
+
+ while (len > 0) {
+ int chunkSize_ = chunkSize > len ? len : chunkSize;
+ int bytes_written = SSL_write(ss->ssl_,
+ buffer_data + off + offset,
+ chunkSize_);
+ bytes_written = ss->HandleSSLError("SSL_write:ClearIn",
+ bytes_written);
+
+ // Written nothing at all
+ if (bytes_written <= 0) {
+ // If we has failed to write first chunk
+ // total_bytes_written can have negative value
+ if (total_bytes_written == 0) {
+ total_bytes_written += bytes_written;
+ }
+ break;
+ }
+
+ total_bytes_written += bytes_written;
+
+ // Written less than planned
+ if (bytes_written < chunkSize_) {
+ break;
+ }
+
+ offset += chunkSize_;
+ len -= chunkSize_;
+ }
- ss->HandleSSLError("SSL_write:ClearIn", bytes_written);
ss->SetShutdownFlags();
- return scope.Close(Integer::New(bytes_written));
+ return scope.Close(Integer::New(total_bytes_written));
}
Something went wrong with that request. Please try again.