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

AST: More efficient way to collect referenced source units #15579

Merged
merged 1 commit into from
Nov 21, 2024
Merged

AST: More efficient way to collect referenced source units #15579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 10 additions & 5 deletions libsolidity/ast/AST.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,18 +130,23 @@ SourceUnitAnnotation& SourceUnit::annotation() const
std::set<SourceUnit const*> SourceUnit::referencedSourceUnits(bool _recurse, std::set<SourceUnit const*> _skipList) const
{
std::set<SourceUnit const*> sourceUnits;
referencedSourceUnits(sourceUnits, _recurse, _skipList);
return sourceUnits;
}

void SourceUnit::referencedSourceUnits(std::set<SourceUnit const*>& _referencedSourceUnits, bool _recurse, std::set<SourceUnit const*>& _skipList) const
{
for (ImportDirective const* importDirective: filteredNodes<ImportDirective>(nodes()))
{
auto const& sourceUnit = importDirective->annotation().sourceUnit;
if (!_skipList.count(sourceUnit))
auto [skipListIt, notOnSkipListYet] = _skipList.insert(sourceUnit);
if (notOnSkipListYet)
{
_skipList.insert(sourceUnit);
sourceUnits.insert(sourceUnit);
_referencedSourceUnits.insert(sourceUnit);
if (_recurse)
sourceUnits += sourceUnit->referencedSourceUnits(true, _skipList);
sourceUnit->referencedSourceUnits(_referencedSourceUnits, true, _skipList);
Copy link
Member

@cameel cameel Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note that passing _skipList by reference technically changes behavior here - the items added in recursive calls now affect the next loop cycle, previously they were discarded.

Not sure if this was intentional, but fortunately looks like it works to our advantage here, because _skipList can only grow and we avoid revisiting the same unit multiple times that way. Could have caused issues if that wasn't the cause though.

EDIT: Just noticed the PR description and it looks like it was fully intentional :)

Copy link
Member

Choose a reason for hiding this comment

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

Even with that there's still some redundancy here: we run this function for every contract and each time the function walks the same import chains again:

solidity/libsolidity/interface/CompilerStack.cpp

Lines 1675 to 1676 in b7b10c8

for (auto const sourceUnit: _contract.contract->sourceUnit().referencedSourceUnits(true))
referencedSources.insert(*sourceUnit->annotation().path);

We could avoid that by storing the list of referenced imports in SourceUnit. Not sure if it's worth it though. Looking at benchmarks, probably not.

}
}
return sourceUnits;
}

ImportAnnotation& ImportDirective::annotation() const
Expand Down
6 changes: 6 additions & 0 deletions libsolidity/ast/AST.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,12 @@ class SourceUnit: public ASTNode, public ScopeOpener
bool experimentalSolidity() const { return m_experimentalSolidity; }

private:
void referencedSourceUnits(
std::set<SourceUnit const*>& _referencedSourceUnits,
bool _recurse,
std::set<SourceUnit const*>& _skipList
) const;

std::optional<std::string> m_licenseString;
std::vector<ASTPointer<ASTNode>> m_nodes;
bool m_experimentalSolidity = false;
Expand Down