You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Justin Erenkrantz <ju...@erenkrantz.com> on 2007/01/02 23:01:02 UTC
[PATCH] Optimized MD5 implementation from OpenSSL
One of the bottlenecks that keeps popping up in Subversion is the speed of
the MD5 checksums. OpenSSL has put in some work to have optimized MD5
implementations and with David's recent work to detect OpenSSL. we can just
defer to their implementations. For AMD64/EMT64 CPUs, we can leverage
their optimized implementation (which goes from ~250MB/sec to ~370MB/sec).
However, the big annoying thing is that we declared apr_md5_ctx_t in
apr_md5.h so it's not an opaque value. Luckily for us though, OpenSSL's
MD5 context is smaller than APR's - so an ugly hack works.
OpenSSL's ctx:
#define MD5_CBLOCK��64
#define MD5_LBLOCK��(MD5_CBLOCK/4)
#define MD5_DIGEST_LENGTH 16
typedef struct MD5state_st
{
MD5_LONG A,B,C,D;
MD5_LONG Nl,Nh;
MD5_LONG data[MD5_LBLOCK];
unsigned int num;
} MD5_CTX;
Our context:
struct apr_md5_ctx_t {
/** state (ABCD) */
apr_uint32_t state[4];
/** number of bits, modulo 2^64 (lsb first) */
apr_uint32_t count[2];
/** input buffer */
unsigned char buffer[64];
/** translation handle�
* ignored if xlate is unsupported
*/
apr_xlate_t *xlate;
};
Using the optimized MD5 implementation tends to shave 5%-10% or so off
Subversion checkout times in rough testing on Solaris 10 AMD64.
The performance win tells me that it's probably worth it to investigate
this further, but if we can't use OpenSSL's MD5 interface, we need to
duplicate their optimized implementations - which I don't really want to do
unless we have to do so.
Thoughts? -- justin
Index: crypto/apr_md5.c
===================================================================
--- crypto/apr_md5.c (revision 491752)
+++ crypto/apr_md5.c (working copy)
@@ -76,6 +76,39 @@
#include <pthread.h>
#endif
+#if defined(APU_HAVE_SSL) && !APR_CHARSET_EBCDIC
+
+#include "openssl/md5.h"
+
+APU_DECLARE(apr_status_t) apr_md5_init(apr_md5_ctx_t *context)
+{
+ MD5_Init(context);
+ return APR_SUCCESS;
+}
+
+APU_DECLARE(apr_status_t) apr_md5_set_xlate(apr_md5_ctx_t *context,
+ apr_xlate_t *xlate)
+{
+ return APR_ENOTIMPL;
+}
+
+APU_DECLARE(apr_status_t) apr_md5_update(apr_md5_ctx_t *context,
+ const void *_input,
+ apr_size_t inputLen)
+{
+ MD5_Update(context, _input, inputLen);
+ return APR_SUCCESS;
+}
+
+APU_DECLARE(apr_status_t) apr_md5_final(unsigned char digest[APR_MD5_DIGESTSIZE],
+ apr_md5_ctx_t *context)
+{
+ MD5_Final(digest, context);
+ return APR_SUCCESS;
+}
+
+#else
+
/* Constants for MD5Transform routine.
*/
@@ -313,24 +346,6 @@
return APR_SUCCESS;
}
-/* MD5 in one step (init, update, final)
- */
-APU_DECLARE(apr_status_t) apr_md5(unsigned char digest[APR_MD5_DIGESTSIZE],
- const void *_input,
- apr_size_t inputLen)
-{
- const unsigned char *input = _input;
- apr_md5_ctx_t ctx;
- apr_status_t rv;
-
- apr_md5_init(&ctx);
-
- if ((rv = apr_md5_update(&ctx, input, inputLen)) != APR_SUCCESS)
- return rv;
-
- return apr_md5_final(digest, &ctx);
-}
-
/* MD5 basic transformation. Transforms state based on block. */
static void MD5Transform(apr_uint32_t state[4], const unsigned char block[64])
{
@@ -452,7 +467,26 @@
(((apr_uint32_t)input[j + 2]) << 16) |
(((apr_uint32_t)input[j + 3]) << 24);
}
+#endif
+/* MD5 in one step (init, update, final)
+ */
+APU_DECLARE(apr_status_t) apr_md5(unsigned char digest[APR_MD5_DIGESTSIZE],
+ const void *_input,
+ apr_size_t inputLen)
+{
+ const unsigned char *input = _input;
+ apr_md5_ctx_t ctx;
+ apr_status_t rv;
+
+ apr_md5_init(&ctx);
+
+ if ((rv = apr_md5_update(&ctx, input, inputLen)) != APR_SUCCESS)
+ return rv;
+
+ return apr_md5_final(digest, &ctx);
+}
+
#if APR_CHARSET_EBCDIC
APU_DECLARE(apr_status_t) apr_MD5InitEBCDIC(apr_xlate_t *xlate)
{
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Graham Leggett <mi...@sharp.fm>.
On Wed, January 3, 2007 9:32 am, Justin Erenkrantz wrote:
> But, again, really, the 'right' thing is to figure out to marshal into
> their API rather than duplicate their implementation. I'm fine with
> an unoptimized fallback, but if OpenSSL is available and they've done
> the hard work to optimize it, I'd like APR-using programs to benefit.
This definitely makes sense.
Regards,
Graham
--
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Issac Goldstand <ma...@beamartyr.net>.
True, but it's used popularly as a generic digesting algorithm, without
any connection to crypto, and people may not consider that when deciding
whether to compile in something that simply looks like SSL support.
Issac
Justin Erenkrantz wrote:
> On 1/2/07, Issac Goldstand <ma...@beamartyr.net> wrote:
>> If you do this, it might be a smart idea to write in the README and/or
>> INSTALL and/or configure hints that it's worthwhile to link against
>> OpenSSL even if you're not planning on using any crypto features, in
>> order to benefit from this.
>
> Well, MD5 *is* crypto. =P -- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/3/07, Roy T. Fielding <ro...@gmail.com> wrote:
> One-way hash functions are not crypto. Just copy the friggin code.
Well, it's in both our and OpenSSL's 'crypto' directory. =)
FWIW, OpenSSL uses Perl scripts at compile-time to generate the
relevant assembler based upon the local environment. So, it's not a
simple cut-and-paste. The starting point is here:
http://cvs.openssl.org/rlog?f=openssl/crypto/md5
Their implementation willl descend into the asm dir and up to the
parent dir with md32_common.h.
*shrug* -- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by "Roy T. Fielding" <ro...@gmail.com>.
On Jan 2, 2007, at 11:56 PM, Justin Erenkrantz wrote:
> On 1/2/07, Issac Goldstand <ma...@beamartyr.net> wrote:
>> If you do this, it might be a smart idea to write in the README
>> and/or
>> INSTALL and/or configure hints that it's worthwhile to link against
>> OpenSSL even if you're not planning on using any crypto features, in
>> order to benefit from this.
>
> Well, MD5 *is* crypto. =P -- justin
One-way hash functions are not crypto. Just copy the friggin code.
....Roy
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/2/07, Issac Goldstand <ma...@beamartyr.net> wrote:
> If you do this, it might be a smart idea to write in the README and/or
> INSTALL and/or configure hints that it's worthwhile to link against
> OpenSSL even if you're not planning on using any crypto features, in
> order to benefit from this.
Well, MD5 *is* crypto. =P -- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Issac Goldstand <ma...@beamartyr.net>.
If you do this, it might be a smart idea to write in the README and/or
INSTALL and/or configure hints that it's worthwhile to link against
OpenSSL even if you're not planning on using any crypto features, in
order to benefit from this.
Justin Erenkrantz wrote:
> But, again, really, the 'right' thing is to figure out to marshal into
> their API rather than duplicate their implementation. I'm fine with
> an unoptimized fallback, but if OpenSSL is available and they've done
> the hard work to optimize it, I'd like APR-using programs to benefit.
> -- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/2/07, Ruediger Pluem <rp...@apache.org> wrote:
> Can we be sure that M5_LONG is always 32bit on all platforms that are
> supported by apr-util?
Well, probably. Ideally, the 'right' solution would be to use their
structure rather than shoe-horning it into our slightly different
structure. For a proof of concept, yah, it works for me. =)
> But wouldn't this also solve our possible license problems with RSA regarding
> MD5, which is currently investigated?
Yes, or rather it would make the problem OpenSSL's and not ours.
> Plus it would relief us from the ugly hack above.
> But regardles of this, would we be able to duplicate their optimized implementation
> without getting any copyright trouble with the openssl guys?
The OpenSSL optimized MD5 implementation is based on code written here:
http://etud.epita.fr/~bevand_m/papers/md5-amd64.html
So, the AMD64 assembly is under public domain (and a valid disclaim of
copyright, so actually is public domain). The OpenSSL folks have done
some tweaking to the assembly to make it more portable. Plus, OpenSSL
is under a modified BSD license, so we can incorporate their code as
long as we attribute properly:
http://www.openssl.org/source/license.html
But, again, really, the 'right' thing is to figure out to marshal into
their API rather than duplicate their implementation. I'm fine with
an unoptimized fallback, but if OpenSSL is available and they've done
the hard work to optimize it, I'd like APR-using programs to benefit.
-- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Ruediger Pluem <rp...@apache.org>.
On 01/02/2007 11:01 PM, Justin Erenkrantz wrote:
>
> However, the big annoying thing is that we declared apr_md5_ctx_t in
> apr_md5.h so it's not an opaque value. Luckily for us though, OpenSSL's
> MD5 context is smaller than APR's - so an ugly hack works.
>
> OpenSSL's ctx:
>
> #define MD5_CBLOCK��64
> #define MD5_LBLOCK��(MD5_CBLOCK/4)
> #define MD5_DIGEST_LENGTH 16
>
> typedef struct MD5state_st
> {
> MD5_LONG A,B,C,D;
> MD5_LONG Nl,Nh;
> MD5_LONG data[MD5_LBLOCK];
> unsigned int num;
> } MD5_CTX;
Can we be sure that M5_LONG is always 32bit on all platforms that are
supported by apr-util?
>
> Our context:
>
> struct apr_md5_ctx_t {
> /** state (ABCD) */
> apr_uint32_t state[4];
> /** number of bits, modulo 2^64 (lsb first) */
> apr_uint32_t count[2];
> /** input buffer */
> unsigned char buffer[64];
> /** translation handle�
> * ignored if xlate is unsupported
> */
> apr_xlate_t *xlate;
> };
>
> Using the optimized MD5 implementation tends to shave 5%-10% or so off
> Subversion checkout times in rough testing on Solaris 10 AMD64.
>
> The performance win tells me that it's probably worth it to investigate
> this further, but if we can't use OpenSSL's MD5 interface, we need to
> duplicate their optimized implementations - which I don't really want to do
> unless we have to do so.
But wouldn't this also solve our possible license problems with RSA regarding
MD5, which is currently investigated?
Plus it would relief us from the ugly hack above.
But regardles of this, would we be able to duplicate their optimized implementation
without getting any copyright trouble with the openssl guys?
Regards
Rüdiger
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Justin Erenkrantz wrote:
> On 1/3/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>
>> >> Now - I thought the public discussion lists were moving twords disolving
>> >> APR-util into smaller libraries, dependent on specific features that
>> >> wouldn't swallow the entire world of unnecessary library functionality
>> >> into every runtime?
>> >
>> > That's part of the discussions that have been here on list, but
>> > frankly, I've never seen anyone say how a dynamic APR would work
>> > reliably. In fact, I think the eventual consensus was that
>> > splitting-off approach could never work at all.
>>
> http://mail-archives.apache.org/mod_mbox/apr-dev/200608.mbox/%3C7edfeeef0608110942h8caf669t96585359a5c61f17@mail.gmail.com%3E
>
> That's how I interpreted the outcome of that thread.
That's not how I interpreted it. I read that it's necessary to load modules
such that...
* global pool
\- DSO's pool
\- DSO's object's pools
Of course, it's possible to create the objects in the DSO's pool, because we
run the cleanups in LIFO order. It's also possible to load it all in the global
pool. A bigger change for 2.0 would be to ensure the global pool IS always one
threadsafe pool, which I'd strongly support.
* global pool
\- DSO's object's pools
\- DSO's pool
and other pool usage layouts all lead to the crashes that Garrett described.
I'd argue this is program design error, and not
But you hint that we would dynaload (for example) the apr-ldap component, or
at least the libldap/liblber libraries. I'm not arguing that.
I'm suggesting libaprldap-2.so would be dynalinked to the user's app. Rather
than just plugging in APR_UTIL_LIBS, a user who wanted ldap plus dbd would use
APR_LDAP_LIBS and APR_DBD_LIBS.
Sure, we would proliferate five or more seperate aprfoo-2.so dso's. Is that
bad? Is that worse than dragging in MB's of code per aprfoo-2.so component,
that aren't needed?
[Snip httpd/svn-centric dialog]
The goal of 2.0 should be to make apr user's (the developer's) experience
less painful. That means a more consistent API, a lighter weight core of
functionality that applies to them, and fewer surprises. At least that's
my humble 2c.
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/3/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> > Here on the list and at every hackathon/ApacheCon for the last few years.
>
> Searching body text of the history for 'merge apr' or 'combine apr' yields
> no results, pointer please. Undocumented hackathon discussions are not
> valid citations.
I didn't say they were decisions (which need to be made on-list), but
discussions. *shrug*
> >> Now - I thought the public discussion lists were moving twords disolving
> >> APR-util into smaller libraries, dependent on specific features that
> >> wouldn't swallow the entire world of unnecessary library functionality
> >> into every runtime?
> >
> > That's part of the discussions that have been here on list, but
> > frankly, I've never seen anyone say how a dynamic APR would work
> > reliably. In fact, I think the eventual consensus was that
> > splitting-off approach could never work at all.
>
> Where is that documented?
http://mail-archives.apache.org/mod_mbox/apr-dev/200608.mbox/%3C7edfeeef0608110942h8caf669t96585359a5c61f17@mail.gmail.com%3E
That's how I interpreted the outcome of that thread.
> Please don't start a thread on "Our agreed way to ..." when in fact there's
> no documented technical discussion, and start a fresh thread to attempt to
> argue that svn or log4cxx really needs to load libldap/liblber.
They load them today, so your argument that we must prevent that in
2.0 is bollocks. Frankly, the idea that APR is used without APR-util
is not supported by any actual evidence. IMHO, it'd make everyone's
life far simpler if we faced reality and combine them into one
library. -- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Justin Erenkrantz wrote:
> On 1/3/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
>> > We've long talked about merging APR and APR-util into one library
>> for 2.0.
>>
>> Pointers?!?
>
> Here on the list and at every hackathon/ApacheCon for the last few years.
Searching body text of the history for 'merge apr' or 'combine apr' yields
no results, pointer please. Undocumented hackathon discussions are not
valid citations.
>> Now - I thought the public discussion lists were moving twords disolving
>> APR-util into smaller libraries, dependent on specific features that
>> wouldn't swallow the entire world of unnecessary library functionality
>> into every runtime?
>
> That's part of the discussions that have been here on list, but
> frankly, I've never seen anyone say how a dynamic APR would work
> reliably. In fact, I think the eventual consensus was that
> splitting-off approach could never work at all.
Where is that documented?
Please don't start a thread on "Our agreed way to ..." when in fact there's
no documented technical discussion, and start a fresh thread to attempt to
argue that svn or log4cxx really needs to load libldap/liblber.
Re: Fwd: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Lucian Adrian Grijincu wrote:
>
> Having a the possibility to give "--disable-ldap --disable-md4
> --disable-xml" as parameters to ./configure or some other
> precompilation routine would be better than having them bundled with
> the library when they are not needed.
You have that, today. It isn't quite a solution.
If your library relies on a system-installed libapr/libaprutil and you
link to the shared (rather than static) libraries, your code will pull
in all these dependencies that are never triggered.
Don't you dare install a --disable-ldap/--disable-xml shared library
into the system location, or you have borked svn, httpd, and other apps
that consume apr-util and *do* require those features.
Rather, if we dump apr-util, splitting it into apr-ldap, apr-db, apr-dbd
etc, then your program would pull in only the dependencies it (may) use.
I said 'may use' because if you call up apr-dbd, you might find it's built
with support for postgresql, mysql, db2, etc. That's OK because if you are
using apr-dbd, you probably mean for your end user to be able to tie into
one of several backend providers that are available. Right?
If you are only supporting one backend, why use apr-dbd in the first place?
But that has no relationship to xml, ldap etc. Since you don't need those
backends, so there is no reason to pull them in.
One last point, there is no reason for you to build static --disable-ldap
libraries, etc. If your module doesn't use the apr_ldap_foo() interfaces,
then those .o modules won't be included in the final binary anyways.
Bill
Fwd: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
On 1/3/07, Justin Erenkrantz <ju...@erenkrantz.com> wrote:
>
> How many apps use APR but not APR-util? I don't know of any. All of
> the consumers I know about use both. So, they're getting it anyway
> today. -- justin
>
Well, I for one am working on a project using APR, but not APR-util.
And I know of a few other projects in my company that use APR but not APR-util.
I haven't checked APR-util build system to see if this is implemented,
but I think having the possibility to select at compile time what
features you want to disable would be best.
E.g. one may need APR, and would find useful the Thread Safe FIFO
bounded queue and Base64 Encoding code from APR-util. The rest is just
useless.
Having a the possibility to give "--disable-ldap --disable-md4
--disable-xml" as parameters to ./configure or some other
precompilation routine would be better than having them bundled with
the library when they are not needed.
something like this was proposed at the beginning, while discussing
APR's mission statement:
http://mail-archives.apache.org/mod_mbox/apr-dev/200011.mbox/raw/%3cPine.LNX.4.21.0011141303060.20293-100000@koj.rkbloom.net%3e
From: rbb@covalent.net
Subject: Re: Mission Statement
> > >
> > > : The Apache Portable Run-time mission is to provide a library of routines
> > > : that allows programmers to write a program once and be able to compile
> > > : it anywhere.
> > >
>I have always had it in my head that it would be nice to get some gui
>stuff in APR at some point, a LONG time from now. If this sort of thing
>is ever added, it would be with compile time switches to shut it off
>completely.
>
>Or, the GUI part could end up a sub-project of APR, something like
>apr-gui. That would still make it a part of the APR project, but
>completely disjoint from APR when it comes to the actual code/libraries.
>
>Ryan
--
Best Regards,
Lucian Adrian Grijincu
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/3/07, William A. Rowe, Jr. <wr...@rowe-clan.net> wrote:
> > We've long talked about merging APR and APR-util into one library for 2.0.
>
> Pointers?!?
Here on the list and at every hackathon/ApacheCon for the last few years.
> Now - I thought the public discussion lists were moving twords disolving
> APR-util into smaller libraries, dependent on specific features that
> wouldn't swallow the entire world of unnecessary library functionality
> into every runtime?
That's part of the discussions that have been here on list, but
frankly, I've never seen anyone say how a dynamic APR would work
reliably. In fact, I think the eventual consensus was that
splitting-off approach could never work at all.
> Forcing every binary that consumes APR to load OpenSSL + OpenLDAP + three
> to seven different DB/SQL implementations + one to three XML implementations
> etc ad nausem is completely blink'n insane, as a long term approach.
*shrug*
How many apps use APR but not APR-util? I don't know of any. All of
the consumers I know about use both. So, they're getting it anyway
today. -- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Justin Erenkrantz wrote:
> On 1/3/07, Joe Orton <jo...@redhat.com> wrote:
>> Making apr_md5_ctx_t opaque would require bumping the APR-util major
>> version either way; if there's enough desire to break API that might as
>> well be the way to go?
Well, I simply encapsulated the SHA1 ctx as Justin did, this code's been
shipping well over a year. It's the basis for a FIPS-consuming httpd/apr
that discarded MD4/MD5 in favor of SHA-1. It's a good short term approach,
and necessary for implementors if OpenSSL 1.1 is ever FIPS-revalidated.
> We've long talked about merging APR and APR-util into one library for 2.0.
Pointers?!?
> Perhaps with the new year, we should just swallow it and do it?
Now - I thought the public discussion lists were moving twords disolving
APR-util into smaller libraries, dependent on specific features that
wouldn't swallow the entire world of unnecessary library functionality
into every runtime?
Forcing every binary that consumes APR to load OpenSSL + OpenLDAP + three
to seven different DB/SQL implementations + one to three XML implementations
etc ad nausem is completely blink'n insane, as a long term approach.
> Take a month or so to clean up any other structures that should be
> opaque and drop all deprecated functions?
+1 to start some movement twords 2.0 APR[-util]. -1 to the approach offered
in the introduction of your proposal :)
> Obviously, we'd still support 1.x branch; but new stuff goes into
> 2.x... (Ideally, we drop 0.9.x support; which is likely okay as I
> don't think httpd intends to do many more httpd 2.0.x releases...)
/shrug - I think feature freezing 0.9.x doesn't mean we can't revisit
outright bugs that are identified, I just presume they won't show up
all that often anymore.
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/3/07, Joe Orton <jo...@redhat.com> wrote:
> Making apr_md5_ctx_t opaque would require bumping the APR-util major
> version either way; if there's enough desire to break API that might as
> well be the way to go?
We've long talked about merging APR and APR-util into one library for 2.0.
Perhaps with the new year, we should just swallow it and do it?
Take a month or so to clean up any other structures that should be
opaque and drop all deprecated functions?
Obviously, we'd still support 1.x branch; but new stuff goes into
2.x... (Ideally, we drop 0.9.x support; which is likely okay as I
don't think httpd intends to do many more httpd 2.0.x releases...)
WDYT? -- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jan 03, 2007 at 08:29:54AM -0800, Justin Erenkrantz wrote:
> On 1/3/07, Joe Orton <jo...@redhat.com> wrote:
> >This would need to have some severe future-proofing to be safe against
> >any change to MD5_CTX in future versions of OpenSSL, e.g. only using it
> >for the specific sizeof() that structure as currently defined. (that
> >would cover the cases where MD5_LONG is currently not 32-bit too)
>
> Well, I think the 'right' solution may be to deploy a new MD5 API that
> takes a pool and we can dynamically allocate the MD5_CTX rather than
> asking the client to allocate it off the stack.
Making apr_md5_ctx_t opaque would require bumping the APR-util major
version either way; if there's enough desire to break API that might as
well be the way to go?
Alternatively, a slightly less hacky way (and perhaps well-defined C
code :) might be to "thunk" between the two structures in each function,
building the MD5_CTX on the stack, with the MD5_CTX ->num in the ->xlate
field. Though that might dilute the performance gain somewhat and adds
little over your original patch; not sure it's any better really.
On balance, +0 to your patch. The reason I ask about the error values
was this comment regarding dropping apr_status_t from the interface in
apr's STATUS:
david: This was rejected for 1.0 following Ben L's comment that
should we ever start using any other form of md5 (e.g.
openssl) then errors would become a distinct possibility.
but if that really can't happen, no big deal...
Regards,
joe
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Jeff Trawick <tr...@gmail.com>.
On 1/3/07, Justin Erenkrantz <ju...@erenkrantz.com> wrote:
> On 1/3/07, Joe Orton <jo...@redhat.com> wrote:
> > 1. is having an ENOTIMPL _set_xlate really an excusable regression?
>
> Yes. We already do that for !APR_HAS_XLATE case, so callers need to
> handle that anyway.
assert(there's no necessary initialization for _set_xlate to perform
when using the OpenSSL MD5 implementation), so shouldn't it be an
empty function that returns APR_SUCCESS?
If the assertion is not known to be correct then we need to research a
bit further, with the goal that we either return APR_SUCCESS (no
initialization needed) or avoid the use of OpenSSL MD5 at build time
when we know the _set_xlate() must be called.
Make sense?
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/3/07, Joe Orton <jo...@redhat.com> wrote:
> This would need to have some severe future-proofing to be safe against
> any change to MD5_CTX in future versions of OpenSSL, e.g. only using it
> for the specific sizeof() that structure as currently defined. (that
> would cover the cases where MD5_LONG is currently not 32-bit too)
Well, I think the 'right' solution may be to deploy a new MD5 API that
takes a pool and we can dynamically allocate the MD5_CTX rather than
asking the client to allocate it off the stack.
Basically, change apr_md5_init() to:
APU_DECLARE(apr_status_t) apr_md5_init(apr_md5_ctx_t **context,
apr_pool_t *pool);
combined with making apr_md5_ctx_t an opaque structure.
If we change its signature, I don't know what we want to do - do we
follow Subversion's example and use 'apr_md5_init2' and friends? Or,
should we open the flood gates for APR 2.0?
> 1. is having an ENOTIMPL _set_xlate really an excusable regression?
Yes. We already do that for !APR_HAS_XLATE case, so callers need to
handle that anyway.
> 2. is it intended that the return values from the OpenSSL MD5_*
> functions are ignored? (can those functions even use the OpenSSL error
> stack and all the mess that entails?)
My look through those functions doesn't indicate any error returns -
just like our own implementation, FWIW.
> 3. does this mean that apr_md5_* can only be guaranteed to work after
> apu_ssl_init() has been called?
AFAICT, looking at the source code, the OpenSSL MD5 code doesn't use
any of the OpenSSL error stack or functions. (It doesn't use any
errors.) So, it shouldn't depend on apu_ssl_init() being called
first. -- justin
Re: [PATCH] Optimized MD5 implementation from OpenSSL
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jan 02, 2007 at 02:01:02PM -0800, Justin Erenkrantz wrote:
> One of the bottlenecks that keeps popping up in Subversion is the speed of
> the MD5 checksums. OpenSSL has put in some work to have optimized MD5
> implementations and with David's recent work to detect OpenSSL. we can just
> defer to their implementations. For AMD64/EMT64 CPUs, we can leverage
> their optimized implementation (which goes from ~250MB/sec to ~370MB/sec).
>
> However, the big annoying thing is that we declared apr_md5_ctx_t in
> apr_md5.h so it's not an opaque value. Luckily for us though,
> OpenSSL's MD5 context is smaller than APR's - so an ugly hack works.
This would need to have some severe future-proofing to be safe against
any change to MD5_CTX in future versions of OpenSSL, e.g. only using it
for the specific sizeof() that structure as currently defined. (that
would cover the cases where MD5_LONG is currently not 32-bit too)
1. is having an ENOTIMPL _set_xlate really an excusable regression?
2. is it intended that the return values from the OpenSSL MD5_*
functions are ignored? (can those functions even use the OpenSSL error
stack and all the mess that entails?)
3. does this mean that apr_md5_* can only be guaranteed to work after
apu_ssl_init() has been called?
joe