You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by masaori335 <gi...@git.apache.org> on 2016/06/23 02:56:28 UTC
[GitHub] trafficserver pull request #733: TS-4324: Allocate 16KB for DATA frame paylo...
GitHub user masaori335 opened a pull request:
https://github.com/apache/trafficserver/pull/733
TS-4324: Allocate 16KB for DATA frame payload exactly
- [TS-4324 comment-15313205](https://issues.apache.org/jira/browse/TS-4324?focusedCommentId=15313205&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15313205)
- Separate buffer for frame header from `ioblock` to use all of allocated `ioblock` for frame payload.
- Call `http2_write_frame_header()` only once. (currently it is called twice)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/masaori335/trafficserver ts-4324
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/733.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #733
----
commit 5210d7a3dfd0e9e4e656d10da7a0d35dbcb654ef
Author: Masaori Koshiba <ma...@apache.org>
Date: 2016-06-21T11:01:58Z
TS-4324: Allocate 16KB for DATA frame payload exactly
- Cleanup Http2Frame
- Separate frame header and frame payload
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/733
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/329/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #733: TS-4324: Allocate 16KB for DATA frame paylo...
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 closed the pull request at:
https://github.com/apache/trafficserver/pull/733
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/733
\U0001f44d
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on the issue:
https://github.com/apache/trafficserver/pull/733
\U0001f44d Looks good
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/733
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/330/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/733
:+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #733: TS-4324: Allocate 16KB for DATA frame paylo...
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/733#discussion_r68175695
--- Diff: proxy/http2/Http2ClientSession.h ---
@@ -94,63 +89,60 @@ class Http2Frame
{
this->ioblock = new_IOBufferBlock();
this->ioblock->alloc(index);
--- End diff --
Updated:)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #733: TS-4324: Allocate 16KB for DATA frame paylo...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/733#discussion_r68171203
--- Diff: proxy/http2/Http2ClientSession.h ---
@@ -94,63 +89,60 @@ class Http2Frame
{
this->ioblock = new_IOBufferBlock();
this->ioblock->alloc(index);
--- End diff --
You should update the comment above this function now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/733
@bryancall Please review as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #733: TS-4324: Allocate 16KB for DATA frame paylo...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/733#discussion_r68171753
--- Diff: proxy/http2/Http2ClientSession.h ---
@@ -65,16 +65,11 @@ class Http2Frame
public:
Http2Frame(const Http2FrameHeader &h, IOBufferReader *r)
{
- this->hdr.cooked = h;
- this->ioreader = r;
- }
-
- Http2Frame(Http2FrameType type, Http2StreamId streamid, uint8_t flags)
- {
- Http2FrameHeader hdr = {0, (uint8_t)type, flags, streamid};
- http2_write_frame_header(hdr, make_iovec(this->hdr.raw));
+ this->hdr = h;
+ this->ioreader = r;
}
+ Http2Frame(Http2FrameType type, Http2StreamId streamid, uint8_t flags) { this->hdr = {0, (uint8_t)type, flags, streamid}; }
--- End diff --
Should initialize ``ioreader`` to NULL here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #733: TS-4324: Allocate 16KB for DATA frame paylo...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/733#discussion_r68171726
--- Diff: proxy/http2/Http2ClientSession.h ---
@@ -94,63 +89,60 @@ class Http2Frame
{
this->ioblock = new_IOBufferBlock();
this->ioblock->alloc(index);
- memcpy(this->ioblock->start(), this->hdr.raw, sizeof(this->hdr.raw));
- this->ioblock->fill(sizeof(this->hdr.raw));
-
- http2_parse_frame_header(make_iovec(this->ioblock->start(), HTTP2_FRAME_HEADER_LEN), this->hdr.cooked);
}
- // Return the writeable buffer space.
+ // Return the writeable buffer space for frame payload
IOVec
write()
{
return make_iovec(this->ioblock->end(), this->ioblock->write_avail());
}
- // Once the frame has been serialized, update the length.
+ // Once the frame has been serialized, update the payload length of frame header.
void
finalize(size_t nbytes)
{
if (this->ioblock) {
ink_assert((int64_t)nbytes <= this->ioblock->write_avail());
this->ioblock->fill(nbytes);
- this->hdr.cooked.length = this->ioblock->size() - HTTP2_FRAME_HEADER_LEN;
- http2_write_frame_header(this->hdr.cooked, make_iovec(this->ioblock->start(), HTTP2_FRAME_HEADER_LEN));
+ this->hdr.length = this->ioblock->size();
}
}
void
xmit(MIOBuffer *iobuffer)
{
- if (ioblock) {
+ // Write frame header
+ uint8_t buf[HTTP2_FRAME_HEADER_LEN];
+ http2_write_frame_header(hdr, make_iovec(buf));
+ iobuffer->write(buf, sizeof(buf));
+
+ // Write frame payload
+ // It could be empty (e.g. SETTINGS frame with ACK flag)
+ if (ioblock && ioblock->read_avail() > 0) {
iobuffer->append_block(this->ioblock.get());
- } else {
- iobuffer->write(this->hdr.raw, sizeof(this->hdr.raw));
}
}
int64_t
size()
{
if (ioblock) {
- return ioblock->size();
+ return HTTP2_FRAME_HEADER_LEN + ioblock->size();
} else {
- return sizeof(this->hdr.raw);
+ return HTTP2_FRAME_HEADER_LEN;
}
}
private:
Http2Frame(Http2Frame &); // noncopyable
Http2Frame &operator=(const Http2Frame &); // noncopyable
- Ptr<IOBufferBlock> ioblock;
IOBufferReader *ioreader;
- union {
- Http2FrameHeader cooked;
- uint8_t raw[HTTP2_FRAME_HEADER_LEN];
- } hdr;
+ Http2FrameHeader hdr; // frame header
+ Ptr<IOBufferBlock> ioblock; // frame payload
--- End diff --
Bikeshedding, but it would be nicer to keep ``ioreader`` down here near ``ioblock`` :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/733
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/222/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/733
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/223/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/733
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/224/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #733: TS-4324: Allocate 16KB for DATA frame payload exac...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/733
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/328/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---