You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ic...@apache.org on 2015/11/05 15:21:13 UTC
svn commit: r1712782 - in /httpd/httpd/trunk/modules/http2: h2_session.c
h2_stream_set.c h2_stream_set.h h2_version.h
Author: icing
Date: Thu Nov 5 14:21:13 2015
New Revision: 1712782
URL: http://svn.apache.org/viewvc?rev=1712782&view=rev
Log:
replacing own stream_set with apr_hash internally
Modified:
httpd/httpd/trunk/modules/http2/h2_session.c
httpd/httpd/trunk/modules/http2/h2_stream_set.c
httpd/httpd/trunk/modules/http2/h2_stream_set.h
httpd/httpd/trunk/modules/http2/h2_version.h
Modified: httpd/httpd/trunk/modules/http2/h2_session.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_session.c?rev=1712782&r1=1712781&r2=1712782&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_session.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_session.c Thu Nov 5 14:21:13 2015
@@ -204,7 +204,7 @@ static int on_data_chunk_recv_cb(nghttp2
if (session->aborted) {
return NGHTTP2_ERR_CALLBACK_FAILURE;
}
- stream = h2_stream_set_get(session->streams, stream_id);
+ stream = h2_session_get_stream(session, stream_id);
if (!stream) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, session->c,
APLOGNO(02919)
@@ -335,7 +335,7 @@ static int on_stream_close_cb(nghttp2_se
if (session->aborted) {
return NGHTTP2_ERR_CALLBACK_FAILURE;
}
- stream = h2_stream_set_get(session->streams, stream_id);
+ stream = h2_session_get_stream(session, stream_id);
if (stream) {
stream_destroy(session, stream, error_code);
}
@@ -379,8 +379,7 @@ static int on_header_cb(nghttp2_session
if (session->aborted) {
return NGHTTP2_ERR_CALLBACK_FAILURE;
}
- stream = h2_stream_set_get(session->streams,
- frame->hd.stream_id);
+ stream = h2_session_get_stream(session, frame->hd.stream_id);
if (!stream) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, session->c,
APLOGNO(02920)
@@ -421,8 +420,8 @@ static int on_frame_recv_cb(nghttp2_sess
switch (frame->hd.type) {
case NGHTTP2_HEADERS: {
int eos;
- h2_stream * stream = h2_stream_set_get(session->streams,
- frame->hd.stream_id);
+ h2_stream * stream = h2_session_get_stream(session,
+ frame->hd.stream_id);
if (stream == NULL) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, session->c,
APLOGNO(02921)
@@ -444,8 +443,8 @@ static int on_frame_recv_cb(nghttp2_sess
break;
}
case NGHTTP2_DATA: {
- h2_stream * stream = h2_stream_set_get(session->streams,
- frame->hd.stream_id);
+ h2_stream * stream = h2_session_get_stream(session,
+ frame->hd.stream_id);
if (stream == NULL) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, session->c,
APLOGNO(02922)
@@ -490,8 +489,8 @@ static int on_frame_recv_cb(nghttp2_sess
have to check the frame type first. */
if ((frame->hd.type == NGHTTP2_DATA || frame->hd.type == NGHTTP2_HEADERS) &&
frame->hd.flags & NGHTTP2_FLAG_END_STREAM) {
- h2_stream * stream = h2_stream_set_get(session->streams,
- frame->hd.stream_id);
+ h2_stream * stream = h2_session_get_stream(session,
+ frame->hd.stream_id);
if (stream != NULL) {
status = h2_stream_write_eos(stream);
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, session->c,
@@ -546,7 +545,7 @@ static int on_send_data_cb(nghttp2_sessi
return NGHTTP2_ERR_CALLBACK_FAILURE;
}
- stream = h2_stream_set_get(session->streams, stream_id);
+ stream = h2_session_get_stream(session, stream_id);
if (!stream) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, APR_NOTFOUND, session->c,
APLOGNO(02924)
@@ -700,7 +699,7 @@ static h2_session *h2_session_create_int
return NULL;
}
- session->streams = h2_stream_set_create(session->pool);
+ session->streams = h2_stream_set_create(session->pool, session->max_stream_count);
session->workers = workers;
session->mplx = h2_mplx_create(c, session->pool, workers);
@@ -769,7 +768,7 @@ void h2_session_destroy(h2_session *sess
session->mplx = NULL;
}
if (session->streams) {
- if (h2_stream_set_size(session->streams)) {
+ if (!h2_stream_set_is_empty(session->streams)) {
ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c,
"h2_session(%ld): destroy, %d streams open",
session->id, (int)h2_stream_set_size(session->streams));
@@ -915,7 +914,7 @@ apr_status_t h2_session_start(h2_session
return status;
}
- stream = h2_stream_set_get(session->streams, 1);
+ stream = h2_session_get_stream(session, 1);
if (stream == NULL) {
status = APR_EGENERAL;
ap_log_rerror(APLOG_MARK, APLOG_ERR, status, session->r,
@@ -1183,7 +1182,7 @@ static ssize_t stream_data_cb(nghttp2_se
(void)ng2s;
(void)buf;
(void)source;
- stream = h2_stream_set_get(session->streams, stream_id);
+ stream = h2_session_get_stream(session, stream_id);
if (!stream) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, session->c,
APLOGNO(02937)
Modified: httpd/httpd/trunk/modules/http2/h2_stream_set.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream_set.c?rev=1712782&r1=1712781&r2=1712782&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_stream_set.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_stream_set.c Thu Nov 5 14:21:13 2015
@@ -19,31 +19,30 @@
#include <apr_strings.h>
#include <httpd.h>
-#include <http_core.h>
-#include <http_connection.h>
#include <http_log.h>
#include "h2_private.h"
-#include "h2_session.h"
#include "h2_stream.h"
-#include "h2_task.h"
#include "h2_stream_set.h"
-#define H2_STREAM_IDX(list, i) ((h2_stream**)(list)->elts)[i]
struct h2_stream_set {
- apr_array_header_t *list;
+ apr_hash_t *hash;
};
-h2_stream_set *h2_stream_set_create(apr_pool_t *pool)
+static unsigned int stream_hash(const char *key, apr_ssize_t *klen)
+{
+ /* we use the "int stream_id" has key, which always odd from
+ * client and even from server. As long as we do not mix them
+ * in one set, snip off the lsb. */
+ return (unsigned int)(*((int*)key)) >> 1;
+}
+
+h2_stream_set *h2_stream_set_create(apr_pool_t *pool, int max)
{
h2_stream_set *sp = apr_pcalloc(pool, sizeof(h2_stream_set));
- if (sp) {
- sp->list = apr_array_make(pool, 100, sizeof(h2_stream*));
- if (!sp->list) {
- return NULL;
- }
- }
+ sp->hash = apr_hash_make_custom(pool, stream_hash);
+
return sp;
}
@@ -52,111 +51,50 @@ void h2_stream_set_destroy(h2_stream_set
(void)sp;
}
-static int h2_stream_id_cmp(const void *s1, const void *s2)
+h2_stream *h2_stream_set_get(h2_stream_set *sp, int stream_id)
{
- return (*((h2_stream **)s1))->id - (*((h2_stream **)s2))->id;
+ return apr_hash_get(sp->hash, &stream_id, sizeof(stream_id));
}
-h2_stream *h2_stream_set_get(h2_stream_set *sp, int stream_id)
+void h2_stream_set_add(h2_stream_set *sp, h2_stream *stream)
{
- /* we keep the array sorted by id, so lookup can be done
- * by bsearch.
- */
- h2_stream key;
- h2_stream *pkey, **ps;
- memset(&key, 0, sizeof(key));
- key.id = stream_id;
- pkey = &key;
- ps = bsearch(&pkey, sp->list->elts, sp->list->nelts,
- sp->list->elt_size, h2_stream_id_cmp);
- return ps? *ps : NULL;
-}
-
-static void h2_stream_set_sort(h2_stream_set *sp)
-{
- qsort(sp->list->elts, sp->list->nelts, sp->list->elt_size,
- h2_stream_id_cmp);
-}
-
-apr_status_t h2_stream_set_add(h2_stream_set *sp, h2_stream *stream)
-{
- h2_stream *existing = h2_stream_set_get(sp, stream->id);
- if (!existing) {
- int last;
- APR_ARRAY_PUSH(sp->list, h2_stream*) = stream;
- /* Normally, streams get added in ascending order if id. We
- * keep the array sorted, so we just need to check of the newly
- * appended stream has a lower id than the last one. if not,
- * sorting is not necessary.
- */
- last = sp->list->nelts - 1;
- if (last > 0
- && (H2_STREAM_IDX(sp->list, last)->id
- < H2_STREAM_IDX(sp->list, last-1)->id)) {
- h2_stream_set_sort(sp);
- }
- }
- return APR_SUCCESS;
-}
-
-h2_stream *h2_stream_set_remove(h2_stream_set *sp, int stream_id)
-{
- int i;
- for (i = 0; i < sp->list->nelts; ++i) {
- h2_stream *s = H2_STREAM_IDX(sp->list, i);
- if (s->id == stream_id) {
- int n;
- --sp->list->nelts;
- n = sp->list->nelts - i;
- if (n > 0) {
- /* Close the hole in the array by moving the upper
- * parts down one step.
- */
- h2_stream **selts = (h2_stream**)sp->list->elts;
- memmove(selts+i, selts+i+1, n * sizeof(h2_stream*));
- }
- return s;
- }
- }
- return NULL;
+ apr_hash_set(sp->hash, &stream->id, sizeof(stream->id), stream);
}
-void h2_stream_set_remove_all(h2_stream_set *sp)
+void h2_stream_set_remove(h2_stream_set *sp, int stream_id)
{
- sp->list->nelts = 0;
+ apr_hash_set(sp->hash, &stream_id, sizeof(stream_id), NULL);
}
int h2_stream_set_is_empty(h2_stream_set *sp)
{
- AP_DEBUG_ASSERT(sp);
- return sp->list->nelts == 0;
+ return apr_hash_count(sp->hash) == 0;
+}
+
+apr_size_t h2_stream_set_size(h2_stream_set *sp)
+{
+ return apr_hash_count(sp->hash);
}
-h2_stream *h2_stream_set_find(h2_stream_set *sp,
- h2_stream_set_match_fn *match, void *ctx)
+typedef struct {
+ h2_stream_set_iter_fn *iter;
+ void *ctx;
+} iter_ctx;
+
+static int hash_iter(void *ctx, const void *key, apr_ssize_t klen,
+ const void *val)
{
- h2_stream *s = NULL;
- int i;
- for (i = 0; !s && i < sp->list->nelts; ++i) {
- s = match(ctx, H2_STREAM_IDX(sp->list, i));
- }
- return s;
+ iter_ctx *ictx = ctx;
+ return ictx->iter(ictx->ctx, (h2_stream*)val);
}
void h2_stream_set_iter(h2_stream_set *sp,
h2_stream_set_iter_fn *iter, void *ctx)
{
- int i;
- for (i = 0; i < sp->list->nelts; ++i) {
- h2_stream *s = H2_STREAM_IDX(sp->list, i);
- if (!iter(ctx, s)) {
- break;
- }
- }
+ iter_ctx ictx;
+ ictx.iter = iter;
+ ictx.ctx = ctx;
+ apr_hash_do(hash_iter, &ictx, sp->hash);
}
-apr_size_t h2_stream_set_size(h2_stream_set *sp)
-{
- return sp->list->nelts;
-}
Modified: httpd/httpd/trunk/modules/http2/h2_stream_set.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream_set.h?rev=1712782&r1=1712781&r2=1712782&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_stream_set.h (original)
+++ httpd/httpd/trunk/modules/http2/h2_stream_set.h Thu Nov 5 14:21:13 2015
@@ -27,25 +27,20 @@ typedef int h2_stream_set_iter_fn(void *
typedef struct h2_stream_set h2_stream_set;
-h2_stream_set *h2_stream_set_create(apr_pool_t *pool);
+h2_stream_set *h2_stream_set_create(apr_pool_t *pool, int max);
void h2_stream_set_destroy(h2_stream_set *sp);
-apr_status_t h2_stream_set_add(h2_stream_set *sp, h2_stream *stream);
+void h2_stream_set_add(h2_stream_set *sp, h2_stream *stream);
h2_stream *h2_stream_set_get(h2_stream_set *sp, int stream_id);
-h2_stream *h2_stream_set_remove(h2_stream_set *sp, int stream_id);
-
-void h2_stream_set_remove_all(h2_stream_set *sp);
+void h2_stream_set_remove(h2_stream_set *sp, int stream_id);
int h2_stream_set_is_empty(h2_stream_set *sp);
apr_size_t h2_stream_set_size(h2_stream_set *sp);
-h2_stream *h2_stream_set_find(h2_stream_set *sp,
- h2_stream_set_match_fn *match, void *ctx);
-
void h2_stream_set_iter(h2_stream_set *sp,
h2_stream_set_iter_fn *iter, void *ctx);
Modified: httpd/httpd/trunk/modules/http2/h2_version.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_version.h?rev=1712782&r1=1712781&r2=1712782&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_version.h (original)
+++ httpd/httpd/trunk/modules/http2/h2_version.h Thu Nov 5 14:21:13 2015
@@ -20,7 +20,7 @@
* @macro
* Version number of the h2 module as c string
*/
-#define MOD_HTTP2_VERSION "1.0.3-DEV"
+#define MOD_HTTP2_VERSION "1.0.4-DEV"
/**
* @macro
@@ -28,7 +28,7 @@
* release. This is a 24 bit number with 8 bits for major number, 8 bits
* for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
*/
-#define MOD_HTTP2_VERSION_NUM 0x010003
+#define MOD_HTTP2_VERSION_NUM 0x010004
#endif /* mod_h2_h2_version_h */
Re: svn commit: r1712782 - in /httpd/httpd/trunk/modules/http2:
h2_session.c h2_stream_set.c h2_stream_set.h h2_version.h
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Nov 5, 2015 at 5:20 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
> But if I only put them in via sth like:
>
> void h2_stream_set_add(h2_stream_set *sp, h2_stream *stream)
> {
> apr_hash_set(sp->hash, &stream->id, sizeof(stream->id), stream);
> }
>
> the (int*) conversion to (void*) and back to (int*) should not hurt. Or?
>
> It's the only way I access the hash and its not visible outside that file...
OK, h2_stream->id is an int member, so it should always be aligned
(provided h2_stream itself is aligned, but that's another story :)
Any char* dereferenced as an integral number makes me nervous, someone
may reuse that hash function someday...
But I agree this is not necessary for now.
Re: svn commit: r1712782 - in /httpd/httpd/trunk/modules/http2: h2_session.c h2_stream_set.c h2_stream_set.h h2_version.h
Posted by Stefan Eissing <st...@greenbytes.de>.
But if I only put them in via sth like:
void h2_stream_set_add(h2_stream_set *sp, h2_stream *stream)
{
apr_hash_set(sp->hash, &stream->id, sizeof(stream->id), stream);
}
the (int*) conversion to (void*) and back to (int*) should not hurt. Or?
It's the only way I access the hash and its not visible outside that file...
//Stefan
> Am 05.11.2015 um 17:07 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Thu, Nov 5, 2015 at 5:03 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Thu, Nov 5, 2015 at 3:21 PM, <ic...@apache.org> wrote:
>>>
>>> -h2_stream_set *h2_stream_set_create(apr_pool_t *pool)
>>> +static unsigned int stream_hash(const char *key, apr_ssize_t *klen)
>>> +{
>>> + /* we use the "int stream_id" has key, which always odd from
>>> + * client and even from server. As long as we do not mix them
>>> + * in one set, snip off the lsb. */
>>> + return (unsigned int)(*((int*)key)) >> 1;
>>
>> This may cause alignment issues, 'key' is not necessarily int or word
>> aligned here.
>> You possibly should go for something like:
>> apr_uint32_r k =
>> (key[0] << 0) |
>> (key[1] << 8) |
>> (key[2] << 16) |
>> (key[3] << 24) ;
>
> Or maybe simply:
> memcpy(&k, key, 4);
> return k;
> ...
Re: svn commit: r1712782 - in /httpd/httpd/trunk/modules/http2:
h2_session.c h2_stream_set.c h2_stream_set.h h2_version.h
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Nov 5, 2015 at 5:03 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Thu, Nov 5, 2015 at 3:21 PM, <ic...@apache.org> wrote:
>>
>> -h2_stream_set *h2_stream_set_create(apr_pool_t *pool)
>> +static unsigned int stream_hash(const char *key, apr_ssize_t *klen)
>> +{
>> + /* we use the "int stream_id" has key, which always odd from
>> + * client and even from server. As long as we do not mix them
>> + * in one set, snip off the lsb. */
>> + return (unsigned int)(*((int*)key)) >> 1;
>
> This may cause alignment issues, 'key' is not necessarily int or word
> aligned here.
> You possibly should go for something like:
> apr_uint32_r k =
> (key[0] << 0) |
> (key[1] << 8) |
> (key[2] << 16) |
> (key[3] << 24) ;
Or maybe simply:
memcpy(&k, key, 4);
return k;
...
Re: svn commit: r1712782 - in /httpd/httpd/trunk/modules/http2:
h2_session.c h2_stream_set.c h2_stream_set.h h2_version.h
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Nov 5, 2015 at 3:21 PM, <ic...@apache.org> wrote:
> Author: icing
> Date: Thu Nov 5 14:21:13 2015
> New Revision: 1712782
>
> URL: http://svn.apache.org/viewvc?rev=1712782&view=rev
> Log:
> replacing own stream_set with apr_hash internally
>
>
> Modified: httpd/httpd/trunk/modules/http2/h2_stream_set.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream_set.c?rev=1712782&r1=1712781&r2=1712782&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/http2/h2_stream_set.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_stream_set.c Thu Nov 5 14:21:13 2015
> @@ -19,31 +19,30 @@
> #include <apr_strings.h>
>
> #include <httpd.h>
> -#include <http_core.h>
> -#include <http_connection.h>
> #include <http_log.h>
>
> #include "h2_private.h"
> -#include "h2_session.h"
> #include "h2_stream.h"
> -#include "h2_task.h"
> #include "h2_stream_set.h"
>
> -#define H2_STREAM_IDX(list, i) ((h2_stream**)(list)->elts)[i]
>
> struct h2_stream_set {
> - apr_array_header_t *list;
> + apr_hash_t *hash;
> };
>
> -h2_stream_set *h2_stream_set_create(apr_pool_t *pool)
> +static unsigned int stream_hash(const char *key, apr_ssize_t *klen)
> +{
> + /* we use the "int stream_id" has key, which always odd from
> + * client and even from server. As long as we do not mix them
> + * in one set, snip off the lsb. */
> + return (unsigned int)(*((int*)key)) >> 1;
This may cause alignment issues, 'key' is not necessarily int or word
aligned here.
You possibly should go for something like:
apr_uint32_r k =
(key[0] << 0) |
(key[1] << 8) |
(key[2] << 16) |
(key[3] << 24) ;
or the other way around depending on byte order (if that matters, probably not).
> +}
Regards,
Yann.