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

[Andrew Dinn]

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