Eliminate redundant ClassFromCP/ClassByName records #4694
Conversation
Whenever a class C contains (in a signature or constant pool entry) a reference to a class with the same name as C, that class reference must resolve to C itself, so no record is necessary. Signed-off-by: Devin Papineau <devinmp@ca.ibm.com>
No beholder needs both a ClassByName and ClassFromCP record identifying the same class, since constant pool entries are also specified by name. The name used in the constant pool at a given index will always match in a successful load, because the beholder will have passed class chain validation. Signed-off-by: Devin Papineau <devinmp@ca.ibm.com>
If a class is first encountered by the symbol validation manager as the result of ClassFromCP, it is necessary to generate both the originally requested ClassFromCP record and a ClassChain validation record. A ClassByName with the same beholder has the same results as ClassFromCP would, and it subsumes ClassChain, so that only ClassByName is needed. The ClassByName record is larger than the ClassFromCP alone, but smaller than the two records would have been together. Signed-off-by: Devin Papineau <devinmp@ca.ibm.com>
| { | ||
| bool operator()(const ClassFromAnyCPIndex &a, const ClassFromAnyCPIndex &b) const | ||
| { | ||
| std::less<TR_OpaqueClassBlock*> lt; |
There was a problem hiding this comment.
Curiosity question: how come you didn't use LexicalOrder here?
There was a problem hiding this comment.
I wanted to keep LexicalOrder file-local in SymbolValidationManager.cpp. It could be useful more generally, but we'd have to figure out where to put it—certainly not in SymbolValidationManager.hpp. I suppose we could also move the implementation of LessClassFromAnyCPIndex::operator() into the cpp file
There was a problem hiding this comment.
I see, I think it's fine as is for now; if ever there's a need for another "less" operator, we can then move the LexicalOrder class somewhere more appropriate.
| @@ -631,7 +631,12 @@ TR::SymbolValidationManager::addClassFromCPRecord(TR_OpaqueClassBlock *clazz, J9 | |||
| if (recordExists(&byName)) | |||
| return true; // already have an equivalent ClassByName | |||
|
|
|||
| bool added = addClassRecord(clazz, new (_region) ClassFromCPRecord(clazz, beholder, cpIndex)); | |||
| bool added; | |||
| if (!isAlreadyValidated(clazz)) // save a ClassChainRecord | |||
There was a problem hiding this comment.
The idea here is that
- because a ClassFromCP+ClassChain is larger than ClassByName, it is better to generate a ClassByName if we've never generated a ClassChain record.
- because ClassFromCP is smaller than ClassByName, it is better to generate a ClassFromCP if we have generated a ClassChain record.
Is that right?
There was a problem hiding this comment.
Yes, especially since in 2 the ClassFromCP will suppress any later ClassByName that would be equivalent
|
jenkins test sanity xlinux,win,plinux,zlinux jdk8,jdk11 |
This series reduces the number of records generated for ClassFromCP and ClassByName: