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

[kalinshi(施慧)]

Thanks Andrew！

“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. 


" 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. 

Regards
Hui

-----邮件原件-----
发件人: Andrew Dinn <adinn at redhat.com> 
发送时间: 2020年10月9日 19:11
收件人: 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 03/10/2020 02:36, kalinshi(施慧) wrote:
> Please help review this JFR issue in 8u backport.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8253837
> 
> Webrev: http://cr.openjdk.java.net/~hshi/8253837/webrev/
Thanks for reporting this problem.

This does not look like a backport request. It is not linked to any related upstream change. It seems like you are just trying to apply a single line patch to jdk8u to fix a problem you have identified in the hash table code.

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.

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 checked upstream to see if there was any precedent for making your suggested change. I looked at the jdk11u code and the change to method equals that you are requesting has actually been made as part of a larger patch for this issue:

  https://bugs.openjdk.java.net/browse/JDK-8231081

Issue 8231081 was raised and fixed in jdk14 and then backported to jdk11u and jdk13u. As you might expect it modifies both JfrSymbolId::equals methods (except that in the upstream code the method is called on_equals). However, it also makes a lot of related changes to deal with leak profiling.

So, we need to decide whether the whole patch is needed or if we just need to fix the hash table code. That means we really need to know more about what is going wrong on jdk8u. Could you provide more details of what caused the problem this patch is trying to fix and, if possible, something that can be run to reproduce the problem?

Also, it might be a good idea for someone involved in applying the original patch or backporting it to jdk11u to take a look at the jdk8u code and see if 1) the whole patch is needed or 2) it would make sense just to backport the changes to the equals methods. Perhaps Jaroslav? Or maybe Goetz/Christoph who seem to have been involved in approving the jdk11u backport

regards,


Andrew Dinn
-----------