Re: 回复: 回复: [8u] RFR(XS) JFR generate same symbol ID for different symbols (high probabily with CDS)(Internet mail)

[Andrew Dinn]

Hi Hui,

Apologies for the delay in responding. I have started looking at the new
webrev and should get back to you with a review tomorrow.

regards,


Andrew Dinn
-----------
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill


On 19/10/2020 15:07, kalinshi(施慧) wrote:
> Hi All,
> 
> Please help review this JFR issue again in 8u. This problem can be easily reproduced with CDS and reproduce steps in JBS.
> 
> Fix includes:
> 1.  Remove map_symbol/map_cstring methods with only hashcode, all lookup must pass symbol or cstring with hashcode.
> 2.  In JfrSymbolId::equals methods, compare symbol reference or cstring contents when hashcodes are same.
> 3.  Rebuild anonymous klass cstring when search its sctring entry id.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8253837
> Webrev: http://cr.openjdk.java.net/~hshi/8253837/webrev_2/
> 
> Regards
> Hui
> 
> -----邮件原件-----
> 发件人: Andrew Dinn <adinn at redhat.com> 
> 发送时间: 2020年10月9日 20:57
> 收件人: kalinshi(施慧) <kalinshi at tencent.com>; jdk8u-dev at openjdk.java.net; Jaroslav Bachorík <jaroslav.bachorik at datadoghq.com>; Langer, Christoph <christoph.langer at sap.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> 主题: Re: 回复: [8u] RFR(XS) JFR generate same symbol ID for different symbols (high probabily with CDS)(Internet mail)
> 
> Hi Hui,
> 
> On 09/10/2020 12:56, kalinshi(施慧) wrote:
>> “I can see that it is possible that two different symbols might 
>> generate the same hash, which is what appears to motivate your patch.
>> However, you have not really explained in what circumstances that 
>> problem will happen. That makes it impossible to know whether the 
>> patch is really needed.“
>>
>> This problem happens when running with CDS and reproduce steps is in 
>> https://bugs.openjdk.java.net/browse/JDK-8253837.
> 
> Apologies for not seeing that explanation. When I opened the JIRA the description tab was not displayed. This now makes a lot more sense.
> 
>> " Also, if it is needed then it is important to know whether this is 
>> the only thing that needs fixing or if more needs to be done. At the 
>> very least I think you will need to patch both JfrSymbolId::equals 
>> methods."
>>
>> I have noticed " bool JfrSymbolId::equals(const char* query, uintptr_t 
>> hash, const CStringEntry* entry) ". Plan is to investigate this after 
>> finding a reproducible case, this code is quite different from JDK11. 
>> In 8u, some caller is from JfrSymbolId::map_cstring(uintptr_t hash) 
>> with NULL query and hash code, while in 11u map_cstring method has 
>> removed and a declaration is left.
> 
> Well, I see what you mean regarding calls via map_cstring. There is no possibility of query and entry->literal() being different strings with the same hashcode when equals is called via map_cstring because it only calls lookup_only() with query == null.
> 
> However, the cstring table has to be updated somewhere and this happens when method id() is called in JfrSymbolId::mark. id() calls lookup_put() with a non null string. Now that looks to me like it can suffer the same problem as the symbol insert. If two symbols can generate the same hash then I don't see why two C strings could not also generate the same hash. That may be unlikely but I don't think it is impossible. So, I think it is going to be necessary to change both equals methods. Do you have any reasoning that explains why a hash collision is not possible?
> 
> I'd also still like to see a response from one of the upstream devs who worked on JDK-8231081 to explain why it needed to make allowance for hash collisions and also to ask if they see any good reason to backport JDK-8231081. If we need to do that anyway then we might as well apply all the required changes in one go. Jaroslav, any comments?
> 
> regards,
> 
> 
> Andrew Dinn
> -----------
> Red Hat Distinguished Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill
> 
>