Mark,
On 6/16/13 12:04 PM, Mark Thomas wrote:
> On 15/06/2013 16:39, Christopher Schultz wrote:
>> All,
>>
>> While looking into a patch for BasicRealm, I noticed that there are two
>> classes that convert byte[] to a hex-encoded string (e.g. byte[] { 1, 2,
>> a, b ] -> "12ab"): HexUtils and MD5Encoder.
>
> That isn't what those methods do. There are 2 output characters for each
> input byte.Sorry... I was trying to type quickly. That should have been "01020a0b" for the given input. >> MD5Encoder only has a single method (encode) and it does exactly what >> HexUtils.toHexString does, except that it only works for exactly 16-byte >> arrays. > > Agreed. > >> I haven't actually written any performance tests, but looking at the >> code it seems that HexUtils.toHexString would execute slightly faster >> for a 16-byte array because of repeated integer multiplication in the loop. > > I'd want to see some hard numbers on both methods before making are > decisions based on performance. I'll write some micro benchmarks and post the results. >> Is there a reason for MD5Encoder to exist? It appears only to be used in >> the following classes: >> >> RealmBase >> DigestAuthenticator >> WebdavServlet > > The check that the input is exactly 16 bytes might be a useful one in > that it confirms other code is behaving as expected. We /could/ make the check and then delegate to the general-purpose method. We'll check the difference between the performance: if a special-purpose method has a significant advantage, it may be worth leaving it separate. How often do we expect these methods to be called? In pathological cases, are these methods called with every request? If we are taking about a once-per-session operation, then performance is less of a concern IMO. >> Although I suppose a specialty 16-byte-only method might be written to >> be slightly faster than the general-purpose HexUtils.toHexString method, >> I'm not sure it's worth having a separate class/method to do it. >> >> Any objections to using HexUtils.toHexString uniformly and removing the >> MD5Encoder class entirely? > > In Tomcat 8, no objections here. > > In Tomcat 7, MD5Encoder will have to remain but be deprecated. Okay. Assuming that I do commit a patch, I'll back-port the switch to HexUtils but not the removal of the MD5Encoder class (as you have done similar patche/back-port pairs when removing classes/methods from TC8). Thanks, -chris
signature.asc
Description: OpenPGP digital signature
