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

Clean up the bytecode comparison scripts #10165

Closed
cameel opened this issue Oct 30, 2020 · 5 comments · Fixed by #10675
Closed

Clean up the bytecode comparison scripts #10165

cameel opened this issue Oct 30, 2020 · 5 comments · Fixed by #10675

Comments

@cameel
Copy link
Member

cameel commented Oct 30, 2020

Extracted from #4698 (comment). Related to #9931.

@ekpyron

That being said, I did only do the minimal amount of work to get it running again in #10039 - it would be nice to clean up the mechanism a bit. I.e. scripts/bytecodecompare/storebytecode.sh for the emscripten case is rather horrible - we could actually just run the solcjs wrapper script from the solc-js repo as solc to run it with the scripts/bytecodecompare/prepare_report.py script instead of using this horrible on-the-fly temporary javascript file produced by the script, etc...

@cameel
Copy link
Member Author

cameel commented Oct 30, 2020

Another small improvement to do here would be replacing the manual stripping of SMTChecker pragmas in https://github.com/ethereum/solidity/blob/v0.7.4/scripts/bytecodecompare/prepare_report.py#L17 with the JSON equivalent of the recently introduced (#10036) --model-checker-engine none option.

@cameel
Copy link
Member Author

cameel commented Dec 14, 2020

@ekpyron As I said on the call today, removing the JS snippet from storebytecode.sh and running solcjs from prepare_report.py works but is sloooooooooooooooooooooooooow. It starts a new node.js process and loads the compiler from scratch for every compiler call.

For this reason I don't think we can get rid of it completely, but we could trim it a bit and convert it into a wrapper for batch compilation that accepts a series of JSON files as input and produces corresponding output JSON files - without making assumptions about what's in it. Writing the input/output out to disk would be less optimal but I think the difference in performance would be negligible and it would allow us to keep the input/output processing logic unified in prepare_report.py.

@ekpyron
Copy link
Member

ekpyron commented Dec 14, 2020

It's fine not to replace the snippet - but did you check exactly what happens? My guess would be that it's not the loading of the compiler, but the issue of node not exiting properly, a.k.a #9297... If you're up for it you can check that and add process.exit(0); unconditionally to the very end of https://github.com/ethereum/solc-js/blob/master/solcjs ...

@ekpyron
Copy link
Member

ekpyron commented Dec 14, 2020

That being said it is a lot of contracts, so keeping one instance of node and the compiler for all of them may still help independently of #9297, so as I said: fine not to change anything about it.

@cameel
Copy link
Member Author

cameel commented Dec 14, 2020

I think that's not it. I'm running node.js 14 and the delay I saw was more like ~1 second per call than the 7 seconds we're seeing there (I made it print a pair of messages around every compiler call). It still adds up over several thousand compilations.

Actually I do see a ~5 second delay when I run this snippet manually on the command line:

node -e "console.log('begin'); console.log(require('./index.js').version()); console.log('end')"

and this is weird because I'm not on node.js 10 and delay is much smaller when I run solcjs from prepare_report.py. But even then I can see that loading the compiler is not instant. It takes it that ~1 second to print the version so it would be that slow even if we fixed #9297.

so as I said: fine not to change anything about it.

I'd prefer to change it actually. The main reason I'm touching it now is that I need to add a bit more functionality there to be able to run solc via CLI too and parse the output. I could limit my changes only to prepare_bytecode.py (because AFAIK there's no way to run CLI via solc-js anyway; solcjs just simulates it using standard JSON internally) but it's bugging me that we have a part of this logic duplicated in the JS snippet. That will lead to them getting out of sync down the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants