From e20b417b8451b96b528efef10627d126283450a6 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Tue, 30 Sep 2014 21:45:15 +0900 Subject: [PATCH] Embed aux_data to nghttp2_outbound_item so that we can save some malloc() calls --- lib/nghttp2_outbound_item.c | 4 --- lib/nghttp2_outbound_item.h | 10 ++++++-- lib/nghttp2_session.c | 33 ++++++++++++++----------- lib/nghttp2_session.h | 12 +++++---- lib/nghttp2_submit.c | 47 +++++++++--------------------------- tests/nghttp2_session_test.c | 14 +++++------ 6 files changed, 52 insertions(+), 68 deletions(-) diff --git a/lib/nghttp2_outbound_item.c b/lib/nghttp2_outbound_item.c index b5db10c1..841d86ab 100644 --- a/lib/nghttp2_outbound_item.c +++ b/lib/nghttp2_outbound_item.c @@ -37,9 +37,6 @@ void nghttp2_outbound_item_free(nghttp2_outbound_item *item) switch(frame->hd.type) { case NGHTTP2_HEADERS: nghttp2_frame_headers_free(&frame->headers); - if(item->aux_data) { - free(((nghttp2_headers_aux_data*)item->aux_data)->data_prd); - } break; case NGHTTP2_PRIORITY: nghttp2_frame_priority_free(&frame->priority); @@ -76,5 +73,4 @@ void nghttp2_outbound_item_free(nghttp2_outbound_item *item) assert(0); } free(item->frame); - free(item->aux_data); } diff --git a/lib/nghttp2_outbound_item.h b/lib/nghttp2_outbound_item.h index 0d1c105b..6d98ddf8 100644 --- a/lib/nghttp2_outbound_item.h +++ b/lib/nghttp2_outbound_item.h @@ -39,18 +39,24 @@ /* Highest weight for PING */ #define NGHTTP2_OB_PING_WEIGHT 302 +/* struct used for HEADERS and PUSH_PROMISE frame */ typedef struct { - nghttp2_data_provider *data_prd; + nghttp2_data_provider data_prd; void *stream_user_data; } nghttp2_headers_aux_data; +/* Additional data which cannot be stored in nghttp2_frame struct */ +typedef union { + nghttp2_headers_aux_data headers; +} nghttp2_aux_data; + typedef struct { + nghttp2_aux_data aux_data; int64_t seq; /* Reset count of weight. See comment for last_cycle in nghttp2_session.h */ uint64_t cycle; void *frame; - void *aux_data; /* Type of |frame|. NGHTTP2_CTRL: nghttp2_frame*, NGHTTP2_DATA: nghttp2_private_data* */ nghttp2_frame_category frame_cat; diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 182a34be..bd8f650b 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -640,7 +640,7 @@ int nghttp2_session_reprioritize_stream int nghttp2_session_add_frame(nghttp2_session *session, nghttp2_frame_category frame_cat, void *abs_frame, - void *aux_data) + nghttp2_aux_data *aux_data) { /* TODO Return error if stream is not found for the frame requiring stream presence. */ @@ -654,7 +654,13 @@ int nghttp2_session_add_frame(nghttp2_session *session, item->frame_cat = frame_cat; item->frame = abs_frame; - item->aux_data = aux_data; + + if(aux_data) { + item->aux_data = *aux_data; + } else { + memset(&item->aux_data, 0, sizeof(item->aux_data)); + } + item->seq = session->next_seq++; /* We use cycle for DATA only */ item->cycle = 0; @@ -1546,7 +1552,7 @@ static int session_prep_frame(nghttp2_session *session, nghttp2_headers_aux_data *aux_data; size_t estimated_payloadlen; - aux_data = (nghttp2_headers_aux_data*)item->aux_data; + aux_data = &item->aux_data.headers; estimated_payloadlen = session_estimate_headers_payload (session, frame->headers.nva, frame->headers.nvlen, @@ -1570,7 +1576,7 @@ static int session_prep_frame(nghttp2_session *session, NGHTTP2_STREAM_FLAG_NONE, &frame->headers.pri_spec, NGHTTP2_STREAM_INITIAL, - aux_data ? aux_data->stream_user_data : NULL); + aux_data->stream_user_data); if(stream == NULL) { return NGHTTP2_ERR_NOMEM; @@ -1580,7 +1586,7 @@ static int session_prep_frame(nghttp2_session *session, (session, frame->hd.stream_id) == 0) { frame->headers.cat = NGHTTP2_HCAT_PUSH_RESPONSE; - if(aux_data && aux_data->stream_user_data) { + if(aux_data->stream_user_data) { nghttp2_stream *stream; stream = nghttp2_session_get_stream(session, frame->hd.stream_id); @@ -1671,7 +1677,7 @@ static int session_prep_frame(nghttp2_session *session, nghttp2_priority_spec pri_spec; size_t estimated_payloadlen; - aux_data = (nghttp2_headers_aux_data*)item->aux_data; + aux_data = &item->aux_data.headers; estimated_payloadlen = session_estimate_headers_payload (session, frame->push_promise.nva, frame->push_promise.nvlen, 0); @@ -1709,8 +1715,7 @@ static int session_prep_frame(nghttp2_session *session, NGHTTP2_STREAM_FLAG_PUSH, &pri_spec, NGHTTP2_STREAM_RESERVED, - aux_data ? - aux_data->stream_user_data : NULL)) { + aux_data->stream_user_data)) { return NGHTTP2_ERR_NOMEM; } break; @@ -2094,11 +2099,11 @@ static int session_after_frame_sent(nghttp2_session *session) return rv; } /* We assume aux_data is a pointer to nghttp2_headers_aux_data */ - aux_data = (nghttp2_headers_aux_data*)item->aux_data; - if(aux_data && aux_data->data_prd) { + aux_data = &item->aux_data.headers; + if(aux_data->data_prd.read_callback) { /* nghttp2_submit_data() makes a copy of aux_data->data_prd */ rv = nghttp2_submit_data(session, NGHTTP2_FLAG_END_STREAM, - frame->hd.stream_id, aux_data->data_prd); + frame->hd.stream_id, &aux_data->data_prd); if(nghttp2_is_fatal(rv)) { return rv; } @@ -2122,10 +2127,10 @@ static int session_after_frame_sent(nghttp2_session *session) return rv; } /* We assume aux_data is a pointer to nghttp2_headers_aux_data */ - aux_data = (nghttp2_headers_aux_data*)item->aux_data; - if(aux_data && aux_data->data_prd) { + aux_data = &item->aux_data.headers; + if(aux_data->data_prd.read_callback) { rv = nghttp2_submit_data(session, NGHTTP2_FLAG_END_STREAM, - frame->hd.stream_id, aux_data->data_prd); + frame->hd.stream_id, &aux_data->data_prd); if(nghttp2_is_fatal(rv)) { return rv; } diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index 45323090..b3c592dd 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -263,10 +263,12 @@ int nghttp2_session_is_my_stream_id(nghttp2_session *session, * |frame_cat| must be either NGHTTP2_CTRL or NGHTTP2_DATA. If the * |frame_cat| is NGHTTP2_CTRL, the |frame| must be a pointer to * nghttp2_frame. If the |frame_cat| is NGHTTP2_DATA, it must be a - * pointer to nghttp2_private_data. |aux_data| is a pointer to the arbitrary - * data. Its interpretation is defined per the type of the frame. When - * this function succeeds, it takes ownership of |frame| and - * |aux_data|, so caller must not free them on success. + * pointer to nghttp2_private_data. |aux_data| specifies additional + * data. Which union member is filled depends on the type of the + * frame. Currently, HEADERS and PUSH_PROMISE frames use headers + * member. When this function succeeds, it takes ownership of + * |frame|. So caller must not free it on success. The content of + * |aux_data| is just copied. * * This function returns 0 if it succeeds, or one of the following * negative error codes: @@ -276,7 +278,7 @@ int nghttp2_session_is_my_stream_id(nghttp2_session *session, */ int nghttp2_session_add_frame(nghttp2_session *session, nghttp2_frame_category frame_cat, - void *abs_frame, void *aux_data); + void *abs_frame, nghttp2_aux_data *aux_data); /* * Adds RST_STREAM frame for the stream |stream_id| with the error diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index 72a516a5..d9d4a0d1 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -48,8 +48,7 @@ static int32_t submit_headers_shared int rv; uint8_t flags_copy; nghttp2_frame *frame = NULL; - nghttp2_data_provider *data_prd_copy = NULL; - nghttp2_headers_aux_data *aux_data = NULL; + nghttp2_aux_data aux_data = {{{{0}, 0}, 0}}; nghttp2_headers_category hcat; if(stream_id == 0) { @@ -58,22 +57,11 @@ static int32_t submit_headers_shared } if(data_prd != NULL && data_prd->read_callback != NULL) { - data_prd_copy = malloc(sizeof(nghttp2_data_provider)); - if(data_prd_copy == NULL) { - rv = NGHTTP2_ERR_NOMEM; - goto fail; - } - *data_prd_copy = *data_prd; - } - if(data_prd || stream_user_data) { - aux_data = malloc(sizeof(nghttp2_headers_aux_data)); - if(aux_data == NULL) { - rv = NGHTTP2_ERR_NOMEM; - goto fail; - } - aux_data->data_prd = data_prd_copy; - aux_data->stream_user_data = stream_user_data; + aux_data.headers.data_prd = *data_prd; } + + aux_data.headers.stream_user_data = stream_user_data; + frame = malloc(sizeof(nghttp2_frame)); if(frame == NULL) { rv = NGHTTP2_ERR_NOMEM; @@ -103,8 +91,7 @@ static int32_t submit_headers_shared hcat, pri_spec, nva_copy, nvlen); - rv = nghttp2_session_add_frame(session, NGHTTP2_CAT_CTRL, frame, - aux_data); + rv = nghttp2_session_add_frame(session, NGHTTP2_CAT_CTRL, frame, &aux_data); if(rv != 0) { nghttp2_frame_headers_free(&frame->headers); @@ -122,8 +109,7 @@ static int32_t submit_headers_shared nghttp2_nv_array_del(nva_copy); fail2: free(frame); - free(aux_data); - free(data_prd_copy); + return rv; } @@ -266,7 +252,7 @@ int32_t nghttp2_submit_push_promise(nghttp2_session *session, uint8_t flags, nghttp2_frame *frame; nghttp2_nv *nva_copy; uint8_t flags_copy; - nghttp2_headers_aux_data *aux_data = NULL; + nghttp2_aux_data aux_data = {{{{0}, 0}, 0}}; int32_t promised_stream_id; int rv; @@ -282,18 +268,11 @@ int32_t nghttp2_submit_push_promise(nghttp2_session *session, uint8_t flags, if(frame == NULL) { return NGHTTP2_ERR_NOMEM; } - if(promised_stream_user_data) { - aux_data = malloc(sizeof(nghttp2_headers_aux_data)); - if(aux_data == NULL) { - free(frame); - return NGHTTP2_ERR_NOMEM; - } - aux_data->data_prd = NULL; - aux_data->stream_user_data = promised_stream_user_data; - } + + aux_data.headers.stream_user_data = promised_stream_user_data; + rv = nghttp2_nv_array_copy(&nva_copy, nva, nvlen); if(rv < 0) { - free(aux_data); free(frame); return rv; } @@ -302,7 +281,6 @@ int32_t nghttp2_submit_push_promise(nghttp2_session *session, uint8_t flags, /* All 32bit signed stream IDs are spent. */ if(session->next_stream_id > INT32_MAX) { - free(aux_data); free(frame); return NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE; @@ -315,11 +293,10 @@ int32_t nghttp2_submit_push_promise(nghttp2_session *session, uint8_t flags, stream_id, promised_stream_id, nva_copy, nvlen); - rv = nghttp2_session_add_frame(session, NGHTTP2_CAT_CTRL, frame, aux_data); + rv = nghttp2_session_add_frame(session, NGHTTP2_CAT_CTRL, frame, &aux_data); if(rv != 0) { nghttp2_frame_push_promise_free(&frame->push_promise); - free(aux_data); free(frame); return rv; diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 83ffd9c2..3b370f04 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -1705,15 +1705,14 @@ void test_nghttp2_session_add_frame(void) MAKE_NV("version", "HTTP/1.1") }; nghttp2_frame *frame; - nghttp2_headers_aux_data *aux_data = - malloc(sizeof(nghttp2_headers_aux_data)); + nghttp2_aux_data aux_data; nghttp2_nv *nva; ssize_t nvlen; memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); callbacks.send_callback = accumulator_send_callback; - memset(aux_data, 0, sizeof(nghttp2_headers_aux_data)); + memset(&aux_data, 0, sizeof(aux_data)); acc.length = 0; user_data.acc = &acc; @@ -1731,7 +1730,7 @@ void test_nghttp2_session_add_frame(void) session->next_stream_id += 2; CU_ASSERT(0 == nghttp2_session_add_frame(session, NGHTTP2_CAT_CTRL, frame, - aux_data)); + &aux_data)); CU_ASSERT(0 == nghttp2_pq_empty(&session->ob_ss_pq)); CU_ASSERT(0 == nghttp2_session_send(session)); CU_ASSERT(NGHTTP2_HEADERS == acc.buf[3]); @@ -2660,10 +2659,9 @@ void test_nghttp2_session_send_headers_start_stream(void) nghttp2_session_callbacks callbacks; nghttp2_frame *frame = malloc(sizeof(nghttp2_frame)); nghttp2_stream *stream; - nghttp2_headers_aux_data *aux_data = - malloc(sizeof(nghttp2_headers_aux_data)); + nghttp2_aux_data aux_data; - memset(aux_data, 0, sizeof(nghttp2_headers_aux_data)); + memset(&aux_data, 0, sizeof(aux_data)); memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); callbacks.send_callback = null_send_callback; @@ -2674,7 +2672,7 @@ void test_nghttp2_session_send_headers_start_stream(void) NGHTTP2_HCAT_REQUEST, NULL, NULL, 0); session->next_stream_id += 2; - nghttp2_session_add_frame(session, NGHTTP2_CAT_CTRL, frame, aux_data); + nghttp2_session_add_frame(session, NGHTTP2_CAT_CTRL, frame, &aux_data); CU_ASSERT(0 == nghttp2_session_send(session)); stream = nghttp2_session_get_stream(session, 1); CU_ASSERT(NGHTTP2_STREAM_OPENING == stream->state);