Skip to content

Commit

Permalink
AST: More efficient way to collect referenced source units
Browse files Browse the repository at this point in the history
Previous implementation of the method SourceUnit::referencedSourceUnits
contained a subtle performance bug. Because the skip list was passed by
value into the recursive call, the dependency graph of the imports were
effectively traversed as if expanded into a full tree, instead of as a
DAG (directed acyclic graph).

An example to illustrate that previously the same source was visited
more than once: Suppose `A.sol` imports `B.sol` and `C.sol` and both of these
import `D.sol`. Previosuly, the method would process `A` by first
recursing into `B` and then `C`. When processing `B`, the source `D` is
processed and then added to the skip list. When the recursion returns
from processing `B`, any changes made to the skip list there were
discarded, so that during processing `C`, the source `D` is not find in
the skip list and processed again.

Now, in most cases the import/dependency graph is probably shallow or does
not contain such diamond-like subgraphs, and the performance is not
affected.
However, for a deeper dependency graph with multiple layers of
diamond-like subgraphs this quickly leads to very bad performance,
because every source unit is visited a number of times equal to the
number of paths by which the source unit is reachable from the root
source unit.

This change seems to shave off *tens* of seconds on **both** legacy and
ir pipeline for `sablier-v2-1.2.0` project.
  • Loading branch information
blishko committed Nov 21, 2024
1 parent b7b10c8 commit 48d40d5
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 5 deletions.
15 changes: 10 additions & 5 deletions libsolidity/ast/AST.cpp
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);
}
}
return sourceUnits;
}

ImportAnnotation& ImportDirective::annotation() const
Expand Down
6 changes: 6 additions & 0 deletions libsolidity/ast/AST.h
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

0 comments on commit 48d40d5

Please sign in to comment.