Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate redundant ClassFromCP/ClassByName records #4694

Merged
merged 3 commits into from
Feb 13, 2019
Merged

Eliminate redundant ClassFromCP/ClassByName records #4694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ef30cd
Skip self-referential ClassFromCP and ClassByName
jdmpapin Nov 7, 2018
b0af003
Find SVM ClassFromCP/ClassByName redundancies
jdmpapin Nov 30, 2018
9f46238
Replace ClassFromCP with ClassByName for fresh classes
jdmpapin Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion runtime/compiler/runtime/SymbolValidationManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ TR::SymbolValidationManager::SymbolValidationManager(TR::Region &region, TR_Reso
_wellKnownClassChainOffsets(NULL),
_symbolValidationRecords(_region),
_alreadyGeneratedRecords(LessSymbolValidationRecord(), _region),
_classesFromAnyCPIndex(LessClassFromAnyCPIndex(), _region),
_symbolToIdMap((SymbolToIdComparator()), _region),
_idToSymbolTable(_region),
_seenSymbolsSet((SeenSymbolsComparator()), _region),
Expand Down Expand Up @@ -396,6 +397,15 @@ TR::SymbolValidationManager::recordExists(TR::SymbolValidationRecord *record)
return _alreadyGeneratedRecords.find(record) != _alreadyGeneratedRecords.end();
}

bool
TR::SymbolValidationManager::anyClassFromCPRecordExists(
TR_OpaqueClassBlock *clazz,
TR_OpaqueClassBlock *beholder)
{
ClassFromAnyCPIndex pair(clazz, beholder);
return _classesFromAnyCPIndex.find(pair) != _classesFromAnyCPIndex.end();
}

void
TR::SymbolValidationManager::appendNewRecord(void *symbol, TR::SymbolValidationRecord *record)
{
Expand Down Expand Up @@ -575,6 +585,10 @@ TR::SymbolValidationManager::addClassByNameRecord(TR_OpaqueClassBlock *clazz, TR
SVM_ASSERT_ALREADY_VALIDATED(this, beholder);
if (isWellKnownClass(clazz))
return true;
else if (clazz == beholder)
return true;
else if (anyClassFromCPRecordExists(clazz, beholder))
return true; // already have an equivalent ClassFromCP
else
return addClassRecordWithChain(new (_region) ClassByNameRecord(clazz, beholder));
}
Expand Down Expand Up @@ -603,12 +617,30 @@ TR::SymbolValidationManager::addProfiledClassRecord(TR_OpaqueClassBlock *clazz)
bool
TR::SymbolValidationManager::addClassFromCPRecord(TR_OpaqueClassBlock *clazz, J9ConstantPool *constantPoolOfBeholder, uint32_t cpIndex)
{
if (inHeuristicRegion())
return true; // to make sure not to modify _classesFromAnyCPIndex

TR_OpaqueClassBlock *beholder = reinterpret_cast<TR_OpaqueClassBlock *>(J9_CLASS_FROM_CP(constantPoolOfBeholder));
SVM_ASSERT_ALREADY_VALIDATED(this, beholder);
if (isWellKnownClass(clazz))
return true;
else if (clazz == beholder)
return true;

ClassByNameRecord byName(clazz, beholder);
if (recordExists(&byName))
return true; // already have an equivalent ClassByName

bool added;
if (!isAlreadyValidated(clazz)) // save a ClassChainRecord
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here is that

  1. because a ClassFromCP+ClassChain is larger than ClassByName, it is better to generate a ClassByName if we've never generated a ClassChain record.
  2. because ClassFromCP is smaller than ClassByName, it is better to generate a ClassFromCP if we have generated a ClassChain record.

Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, especially since in 2 the ClassFromCP will suppress any later ClassByName that would be equivalent

added = addClassRecordWithChain(new (_region) ClassByNameRecord(clazz, beholder));
else
return addClassRecord(clazz, new (_region) ClassFromCPRecord(clazz, beholder, cpIndex));
added = addClassRecord(clazz, new (_region) ClassFromCPRecord(clazz, beholder, cpIndex));

if (added)
_classesFromAnyCPIndex.insert(ClassFromAnyCPIndex(clazz, beholder));

return added;
}

bool
Expand Down
32 changes: 32 additions & 0 deletions runtime/compiler/runtime/SymbolValidationManager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,7 @@ class SymbolValidationManager
bool abandonRecord(TR::SymbolValidationRecord *record);

bool recordExists(TR::SymbolValidationRecord *record);
bool anyClassFromCPRecordExists(TR_OpaqueClassBlock *clazz, TR_OpaqueClassBlock *beholder);
void appendNewRecord(void *symbol, TR::SymbolValidationRecord *record);
void appendRecordIfNew(void *symbol, TR::SymbolValidationRecord *record);

Expand Down Expand Up @@ -848,6 +849,37 @@ class SymbolValidationManager
typedef TR::typed_allocator<J9ClassLoader*, TR::Region&> LoaderAllocator;
typedef std::vector<J9ClassLoader*, LoaderAllocator> LoaderVector;
LoaderVector _loadersOkForWellKnownClasses;

// Remember which classes have already been found in which beholders'
// constant pools, regardless of CP index. If a class C has already been
// found in the constant pool of class D (producing a ClassFromCP record),
// then ClassByName(C, D) is redundant.
struct ClassFromAnyCPIndex
{
TR_OpaqueClassBlock *clazz;
TR_OpaqueClassBlock *beholder;

ClassFromAnyCPIndex(TR_OpaqueClassBlock *clazz, TR_OpaqueClassBlock *beholder)
: clazz(clazz), beholder(beholder) { }
};

struct LessClassFromAnyCPIndex
{
bool operator()(const ClassFromAnyCPIndex &a, const ClassFromAnyCPIndex &b) const
{
std::less<TR_OpaqueClassBlock*> lt;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curiosity question: how come you didn't use LexicalOrder here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (lt(a.clazz, b.clazz))
return true;
else if (lt(b.clazz, a.clazz))
return false;
else
return lt(a.beholder, b.beholder);
}
};

typedef TR::typed_allocator<ClassFromAnyCPIndex, TR::Region&> ClassFromAnyCPIndexAlloc;
typedef std::set<ClassFromAnyCPIndex, LessClassFromAnyCPIndex, ClassFromAnyCPIndexAlloc> ClassFromAnyCPIndexSet;
ClassFromAnyCPIndexSet _classesFromAnyCPIndex;
};

}
Expand Down