-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Comments
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) |
@ekpyron As I said on the call today, removing the JS snippet from 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 |
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 |
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. |
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
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 |
Extracted from #4698 (comment). Related to #9931.
@ekpyron
The text was updated successfully, but these errors were encountered: