On 10/8/19 6:31 AM, Claes Redestad wrote:
Hi, webrev.02 looks good to me.
webrev.02 looks fine to me as well.
I think the performance results makes sense since avoiding a volatile store (and the potentially expensive store barriers this involves) should be the main benefit. Adding a branch to avoid storing null would help partially, but not hot Methods. Pre-existing issue, but it's somewhat weird that we have two assignments outside the package-private constructor: adding root and methodAccessor to the constructor would make more immediate sense to me, since we do the same thing at the only two callsites: Method res = new Method(clazz, name, parameterTypes, returnType, exceptionTypes, modifiers, slot, signature, annotations, parameterAnnotations, annotationDefault); res.root = root; res.methodAccessor = methodAccessor; -> Method res = new Method(clazz, name, parameterTypes, returnType, exceptionTypes, modifiers, slot, signature, annotations, parameterAnnotations, annotationDefault, root, methodAccessor); This package-private constructor could also be made private. My guess is there was some time when this was used from outside Method and changing it was deemed unsavory..
The parameters of the Method constructor are the content from a ClassFile to represent a method whereas root and methodAccessor fields are not part of it. The current implementation allocates a Method instance and sets the fields individually instead of calling the constructor probably due to bootstrapping.
I suggest to keep it as it is. OTOH making the constructor private is fine. Mandy
