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.

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

Performance issues of solc --abi --via-ir #14917

Closed
klkvr opened this issue Mar 6, 2024 · 6 comments · Fixed by #14926
Closed

Performance issues of solc --abi --via-ir #14917

klkvr opened this issue Mar 6, 2024 · 6 comments · Fixed by #14926
Labels
bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. performance 🐎

Comments

@klkvr
Copy link

klkvr commented Mar 6, 2024

Description

It seems that requesting only ABI as output significantly speeds up compilation process which is useful when you don't need anything besides ABI or want to determine files which should be compiled depending on the contracts ABI.

Speed up in case of solc --bin --abi vs solc --abi is great, without --bin it is ~20 times faster on the project I am experimenting with.

However, for some reason solc --abi --via-ir runs much longer than solc --abi or solc --bin. I assume it's because of certain compilation steps not being skipped in IR pipeline which are getting skipped in default compilation mode.

@klkvr klkvr added the bug 🐛 label Mar 6, 2024
@klkvr
Copy link
Author

klkvr commented Mar 6, 2024

It seems that this is not the case when compiling with --standard-json with only "abi" in outputSelection.*.*

@cameel
Copy link
Member

cameel commented Mar 7, 2024

That's odd. Looking at how CommandLineInterface decides whether to perform compilation, --abi is not one of the outputs that would trigger full compilation. And actually producing the ABI is marked as requiring only analysis in CompilerStack. So for both IR and legacy pipeline it should stop at analysis and analysis is the same in both cases. I'd expect any potential slowdown to be in code generation or, much more likely, optimization instead.

How much slowdown are you seeing? Is the repro big enough that the slowdown is not just caused by some small constant factors?

@klkvr
Copy link
Author

klkvr commented Mar 7, 2024

For the same command with different flags I am getting those results:
solc ... --abi: 1.32s
solc ... --via-ir: 162.409s
solc ... --via-ir --abi: 159.849s

Could it be the case that --via-ir always automatically triggers compilation? Sounds like this comment is about this issue:

// TODO: Perhaps we should not compile unless requested

@cameel
Copy link
Member

cameel commented Mar 8, 2024

Could it be the case that --via-ir always automatically triggers compilation?

Well, your numbers show that it clearly must be triggering compilation in this case, but it does not seem to be due to the code in this spot at least. --via-ir corresponds to the m_options.output.viaIR flag and that is not used in the condition. The conditions there are correct. It must be something further down the stack.

Sounds like this comment is about this issue

This comment is outdated. We should remove it. Looking at the original context, we used to just unconditionally compile everything:

try
{
for (auto const& sourceCode: m_sourceCodes)
m_compiler.addSource(sourceCode.first, sourceCode.second);
// TODO: Perhaps we should not compile unless requested
m_compiler.compile(m_args["optimize"].as<bool>());
}

Nowadays we decide based on the output flags whether to compile so that TODO has actually been fulfilled.

@cameel cameel added performance 🐎 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. labels Mar 8, 2024
@cameel
Copy link
Member

cameel commented Mar 8, 2024

Looks like this condition is probably the culprit:

if (m_viaIR || m_generateIR)

@cameel
Copy link
Member

cameel commented Mar 8, 2024

After figuring out what causes it the fix is actually trivial so I created a PR: #14926.

And this indeed does not affect Standard JSON - which in unexpected since the fix is in CompilerStack, which is used in that mode too. Turns out that CLI always runs CompilerStack and uses enableEvmBytecodeGeneration() and enableIRGeneration() to tell it whether compilation is needed (which I think is the more correct approach) while StandardCompiler completely skips compile() when no binary output is requested.

We should really unify the two, because it results in just such odd differences in behavior, but for now I stopped at just fixing the issue at hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. must have eventually Something we consider essential but not enough to prevent us from releasing Solidity 1.0 without it. performance 🐎
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants