You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2015/11/05 17:03:12 UTC
Re: svn commit: r1712782 - in /httpd/httpd/trunk/modules/http2:
h2_session.c h2_stream_set.c h2_stream_set.h h2_version.h
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.
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;
...