You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Andrew Stitcher <as...@apache.org> on 2012/12/07 00:10:21 UTC
Review Request: Avoid using ntohl()/ntohs()/htonl()/htons()
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8385/
-----------------------------------------------------------
Review request for qpid, Cliff Jansen and Rafael Schloming.
Description
-------
Removed the use of [nh]to[hn][ls] in the ANSI only part of the code as they are part of BSD sockets API not in ANSI C.
There is now some duplication of code which should be removed, but nothing serious in my opinion.
This addresses bug PROTON-121.
https://issues.apache.org/jira/browse/PROTON-121
Diffs
-----
/proton/trunk/proton-c/src/codec/codec.c 1417656
/proton/trunk/proton-c/src/framing/framing.c 1417656
Diff: https://reviews.apache.org/r/8385/diff/
Testing
-------
Compiled under Fedora and run proton-test against the built code.
Thanks,
Andrew Stitcher
Re: Review Request: Avoid using ntohl()/ntohs()/htonl()/htons()
Posted by Andrew Stitcher <as...@apache.org>.
> On Dec. 7, 2012, 9:19 a.m., Cliff Jansen wrote:
> > This looks good from a portability perspective. But it must present a performance penalty on big endian hardware, though I can't quantify the amount. Are these functions not available on some platform?
>
> Andrew Stitcher wrote:
> I'd actually be surprised if there was much/any performance problem here since the new functions are inlined and the compiler should be able to do just as good a job. Asa data point this is what we do in qpid and there is no slowdown there.
>
> Another point: the previous code can perform unaligned memory access, which can be a problem on some platforms, whereas this code avoids this issue.
I've done some simple benchmarking with an a modified version of the send/recv examples (modified to send/receive multiple messages).
Testing this change against an equivalent trunk version shows a small speed up of between 1-5% in nearly every case, the worst I saw in this test was a 0.04% slow down. I have no explanation for any speed up due to this code, but I think this test shows the change is small enough to not be noticeable under usual circumstances.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8385/#review14146
-----------------------------------------------------------
On Dec. 6, 2012, 11:10 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8385/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 11:10 p.m.)
>
>
> Review request for qpid, Cliff Jansen and Rafael Schloming.
>
>
> Description
> -------
>
> Removed the use of [nh]to[hn][ls] in the ANSI only part of the code as they are part of BSD sockets API not in ANSI C.
>
> There is now some duplication of code which should be removed, but nothing serious in my opinion.
>
>
> This addresses bug PROTON-121.
> https://issues.apache.org/jira/browse/PROTON-121
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/src/codec/codec.c 1417656
> /proton/trunk/proton-c/src/framing/framing.c 1417656
>
> Diff: https://reviews.apache.org/r/8385/diff/
>
>
> Testing
> -------
>
> Compiled under Fedora and run proton-test against the built code.
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: Avoid using ntohl()/ntohs()/htonl()/htons()
Posted by Andrew Stitcher <as...@apache.org>.
> On Dec. 7, 2012, 9:19 a.m., Cliff Jansen wrote:
> > This looks good from a portability perspective. But it must present a performance penalty on big endian hardware, though I can't quantify the amount. Are these functions not available on some platform?
I'd actually be surprised if there was much/any performance problem here since the new functions are inlined and the compiler should be able to do just as good a job. Asa data point this is what we do in qpid and there is no slowdown there.
Another point: the previous code can perform unaligned memory access, which can be a problem on some platforms, whereas this code avoids this issue.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8385/#review14146
-----------------------------------------------------------
On Dec. 6, 2012, 11:10 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8385/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 11:10 p.m.)
>
>
> Review request for qpid, Cliff Jansen and Rafael Schloming.
>
>
> Description
> -------
>
> Removed the use of [nh]to[hn][ls] in the ANSI only part of the code as they are part of BSD sockets API not in ANSI C.
>
> There is now some duplication of code which should be removed, but nothing serious in my opinion.
>
>
> This addresses bug PROTON-121.
> https://issues.apache.org/jira/browse/PROTON-121
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/src/codec/codec.c 1417656
> /proton/trunk/proton-c/src/framing/framing.c 1417656
>
> Diff: https://reviews.apache.org/r/8385/diff/
>
>
> Testing
> -------
>
> Compiled under Fedora and run proton-test against the built code.
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: Avoid using ntohl()/ntohs()/htonl()/htons()
Posted by Andrew Stitcher <as...@redhat.com>.
On Fri, 2012-12-07 at 08:17 -0500, Hiram Chirino wrote:
> Wouldn't it be cleaner to implement those functions as macros for the
> platforms that don't define it?
I think this is a misunderstanding - The platform in question is *ANSI
C* and this does not define any network to host byte order conversion
functionality. ntohl() etc. are defined by the Sockets API.
The proton engine code is intended to be as absolutely portable as
possible and not depend on anything beyond ANSI C. This will allow it to
be used anywhere that ANSI C is available[1].
Additionally as I note in the review comments the use of the
byteswapping APIs is problematic as it can cause unintended unaligned
memory accesses - even on the i386 architecture where this is allowed it
is slower, on other architectures, I think for example ARM, it will
cause a bus error.
Andrew
[1] I know that it's turned out that using ANSI C99 has not turned out
as portable as we originally thought, but who'd have thought that
Microsoft would ignore a C standard for over 10 years.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: Review Request: Avoid using ntohl()/ntohs()/htonl()/htons()
Posted by Hiram Chirino <hi...@hiramchirino.com>.
Wouldn't it be cleaner to implement those functions as macros for the
platforms that don't define it?
On Fri, Dec 7, 2012 at 4:19 AM, Cliff Jansen <cl...@gmail.com> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8385/#review14146
> -----------------------------------------------------------
>
>
> This looks good from a portability perspective. But it must present a
> performance penalty on big endian hardware, though I can't quantify the
> amount. Are these functions not available on some platform?
>
> - Cliff Jansen
>
>
> On Dec. 6, 2012, 11:10 p.m., Andrew Stitcher wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/8385/
> > -----------------------------------------------------------
> >
> > (Updated Dec. 6, 2012, 11:10 p.m.)
> >
> >
> > Review request for qpid, Cliff Jansen and Rafael Schloming.
> >
> >
> > Description
> > -------
> >
> > Removed the use of [nh]to[hn][ls] in the ANSI only part of the code as
> they are part of BSD sockets API not in ANSI C.
> >
> > There is now some duplication of code which should be removed, but
> nothing serious in my opinion.
> >
> >
> > This addresses bug PROTON-121.
> > https://issues.apache.org/jira/browse/PROTON-121
> >
> >
> > Diffs
> > -----
> >
> > /proton/trunk/proton-c/src/codec/codec.c 1417656
> > /proton/trunk/proton-c/src/framing/framing.c 1417656
> >
> > Diff: https://reviews.apache.org/r/8385/diff/
> >
> >
> > Testing
> > -------
> >
> > Compiled under Fedora and run proton-test against the built code.
> >
> >
> > Thanks,
> >
> > Andrew Stitcher
> >
> >
>
>
--
**
*Hiram Chirino*
*Engineering | Red Hat, Inc.*
*hchirino@redhat.com <hc...@redhat.com> | fusesource.com | redhat.com*
*skype: hiramchirino | twitter: @hiramchirino<http://twitter.com/hiramchirino>
*
*blog: Hiram Chirino's Bit Mojo <http://hiramchirino.com/blog/>*
Re: Review Request: Avoid using ntohl()/ntohs()/htonl()/htons()
Posted by Cliff Jansen <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8385/#review14146
-----------------------------------------------------------
This looks good from a portability perspective. But it must present a performance penalty on big endian hardware, though I can't quantify the amount. Are these functions not available on some platform?
- Cliff Jansen
On Dec. 6, 2012, 11:10 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8385/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 11:10 p.m.)
>
>
> Review request for qpid, Cliff Jansen and Rafael Schloming.
>
>
> Description
> -------
>
> Removed the use of [nh]to[hn][ls] in the ANSI only part of the code as they are part of BSD sockets API not in ANSI C.
>
> There is now some duplication of code which should be removed, but nothing serious in my opinion.
>
>
> This addresses bug PROTON-121.
> https://issues.apache.org/jira/browse/PROTON-121
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/src/codec/codec.c 1417656
> /proton/trunk/proton-c/src/framing/framing.c 1417656
>
> Diff: https://reviews.apache.org/r/8385/diff/
>
>
> Testing
> -------
>
> Compiled under Fedora and run proton-test against the built code.
>
>
> Thanks,
>
> Andrew Stitcher
>
>
Re: Review Request: Avoid using ntohl()/ntohs()/htonl()/htons()
Posted by Cliff Jansen <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8385/#review14171
-----------------------------------------------------------
Ship it!
Ship It!
- Cliff Jansen
On Dec. 6, 2012, 11:10 p.m., Andrew Stitcher wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8385/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2012, 11:10 p.m.)
>
>
> Review request for qpid, Cliff Jansen and Rafael Schloming.
>
>
> Description
> -------
>
> Removed the use of [nh]to[hn][ls] in the ANSI only part of the code as they are part of BSD sockets API not in ANSI C.
>
> There is now some duplication of code which should be removed, but nothing serious in my opinion.
>
>
> This addresses bug PROTON-121.
> https://issues.apache.org/jira/browse/PROTON-121
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/src/codec/codec.c 1417656
> /proton/trunk/proton-c/src/framing/framing.c 1417656
>
> Diff: https://reviews.apache.org/r/8385/diff/
>
>
> Testing
> -------
>
> Compiled under Fedora and run proton-test against the built code.
>
>
> Thanks,
>
> Andrew Stitcher
>
>