Re: Code Review Request: 7171415: java.net.URI.equals/hashCode not consistent for some URIs

Wed, 09 Jan 2013 02:56:10 -0800 

Kurchi,
It is surprising that this bug has remained unnoticed for so long. It effectively breaks the j.l.Object.hashCode/equals contract, "If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result." 


I think chartAt may be the lesser of two evils here, it is what equals 
is using too, and replicating the simple logic String.hashCode is using. 
Vitaly is right, the hashCode does not strictly need to be constructed 
using String.hashCode, but it seems reasonable to simply replicate this 
simple logic, rather than trying to work out an alternative.


-Chris.

On 09/01/2013 04:29, Vitaly Davidovich wrote:

Yes, I also thought about range checks not being eliminated if using
charAt() but I guess that depends on how smart the JIT is - if charAt is
inlined there's technically enough info there for the compiler to see
that range checks aren't needed.  Whether that happens or not I haven't
checked.  toCharArray brings us back to having allocations unless,
again, EA helps out.  I think a microbenchmark would help here (along
with verbose GC logging) to see which is better if this is a concern.

Why do you say you need to duplicate String.hashCode to be consistent
with what people are using already? As long as the hash quality is at
least as good as today (or not significantly worse) shouldn't you be
able to change the impl? If someone's relying on specific value for some
input then their code is broken.  Besides, doing toUpper will change the
hash for URIs with % anyway.  Perhaps I misunderstood your point though ...

Vitaly

Sent from my phone

On Jan 8, 2013 11:04 PM, "Kurchi Subhra Hazra"
<kurchi.subhra.ha...@oracle.com <mailto:kurchi.subhra.ha...@oracle.com>>
wrote:

    On 1/8/13 6:55 PM, Vitaly Davidovich wrote:


    Also, I'm not sure how hot this method is in practice but
    allocating StringBuilder seems a bit heavy (maybe it gets
    partially escape analyzed out though).  Can you instead loop over
    all the chars in the string and build up the hash code as you go
    along? If you see a % then you handle next 2 chars specially, like
    you do now.  Or are you trying to take advantage of String
    intrinsic support in the JIT? I guess if perf is a concern you can
    write a micro benchmark comparing the approaches ...


    That did occur to me, but I guess we have to be consistent with the
    value that people have already been using, and that means I have
    to duplicate the code in String.hashCode() (that is what the
    original implementation was calling) - I was trying to avoid that. Also,
    String.hashCode() uses its internal char[] - whereas charAt() will
    involve several additional bound checks - but
    using toCharArray() may be better. Let me take another look at this,
    and get back with another webrev.


    Sent from my phone

    On Jan 8, 2013 9:45 PM, "Vitaly Davidovich" <vita...@gmail.com
    <mailto:vita...@gmail.com>> wrote:

        Hi Kurchi,

        In the hash method, I suggest you move handling of strings
        with % into a separate method to keep the hash method small
        for common case (no %).  Otherwise, there's a chance this
        method won't get inlined anymore due to its (newly increased)
        size.


    - Yep, will do.


        Also, I realize toLower does the same thing, but why does
        toUpper return an int whereas it's really a char? Might be
        cleaner to declare return type as char and do the cast inside
        the method as needed.


    - I followed the format of toLower(). But I agree this way it will
    be cleaner.

    Thanks a lot,
    Kurchi




        Thanks

        Sent from my phone

        On Jan 8, 2013 8:20 PM, "Kurchi Hazra"
        <kurchi.subhra.ha...@oracle.com
        <mailto:kurchi.subhra.ha...@oracle.com>> wrote:

            Hi,

                According to RFC 3986[1], hexadecimal digits encoded
            by a '%' should be case-insensitive, for example,%A2 and
            %a2 should be
            considered equal. Although, URI.equals() does take this
            into consideration, the implementation of URI.hashCode()
            does not and
            returns different hashcodes for two URIs that are similar
            in all respects except for the case of the percent-encoded
            hexadecimal digits.
            This fix attempts to construct a normalized string from
            the string representing a component before calculating its
            hashCode.
            I converted to upper case for the normalization(and not
            lower case) as required by [1].

                For testing the fix, I added an additional test
            scenario to an existing test
            (jdk/test/java/net/URI/Test.java). While I was there, I
            also made
            minor changes to the test so that it does not produce
            rawtype and other lint warnings.

            Bug:
            http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7171415
            Webrev:
            http://cr.openjdk.java.net/~khazra/7171415/webrev.00/
            <http://cr.openjdk.java.net/%7Ekhazra/7171415/webrev.00/>

            URI.compareTo() still suffers from the same problem - I am
            not sure if it should be dealt with as a separate bug.

            Thanks,
            Kurchi

            [1] http://tools.ietf.org/html/rfc3986#section-6.2.2.1

























					Reply via email to