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

Insert reportedVersion in list.json #21

Open
wants to merge 5 commits into
base: gh-pages
Choose a base branch
from
Open

Conversation

axic
Copy link
Member

@axic axic commented Jun 27, 2018

@fanatid
Copy link

fanatid commented Jun 27, 2018

@axic try with execSync, example:

const cp = require('child_process')
console.log(cp.execSync(`node -e "console.log(require('./bin/soljson-v0.3.1-nightly.2016.4.7+commit.54bc2a').cwrap('version', 'string', [])())"`).toString())

@axic
Copy link
Member Author

axic commented Jun 27, 2018

Thanks! That was my idea for workaround too, but would be nice to figure out what is leaking memory.

@axic axic force-pushed the reported-version branch from 5de8046 to 14cf777 Compare June 27, 2018 12:28
@axic
Copy link
Member Author

axic commented Jun 27, 2018

@fanatid @chriseth can you review?

package.json Outdated Show resolved Hide resolved
@fanatid
Copy link

fanatid commented Jun 27, 2018

29min in Travis -- it's crazy. Also, do you need to run npm run update locally every time on adding new binary?

@axic
Copy link
Member Author

axic commented Jun 27, 2018

Also, do you need to run npm run update locally every time on adding new binary?

Yeah, but it is only done on releases and for every nightly.

@fanatid
Copy link

fanatid commented Jun 27, 2018

If Travis should only verify that files is OK, we can speed-up it with matrix build. But if you are ok with 30min on CI, changes is not worth it.
Regarding running it locally, if you need I'd happy push PR with code for multi-process update. Let me know.

@axic
Copy link
Member Author

axic commented Jun 29, 2018

The list must be updated before merging.

@axic axic force-pushed the reported-version branch from 14cf777 to 75d4816 Compare July 30, 2018 23:06
update Outdated
// NOTE: should be using this, but it leaks memory
// return solc(require(path.join(__dirname, '/bin', file))).version()
const filename = path.join(__dirname, '/bin', file)
return cp.execSync(`/usr/bin/env node -e "console.log(require('${filename}').cwrap('version', 'string', [])())"`).toString().trim()
Copy link
Member Author

Choose a reason for hiding this comment

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

An added side-effect of this is it would fail if the binary can't be loaded or is missing a the version function (e.g. incompatible).

Copy link
Member

Choose a reason for hiding this comment

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

This would be a problem because we have one executable ( soljson-v0.4.1-nightly.2016.9.9+commit.79867f49.js) that crashes when you run it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lol what? Well, there can be an exception.

Copy link
Member

Choose a reason for hiding this comment

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

Well, we could, but I'm still not sure if running the executables while generating the list is such a good idea. It's slow enough already and we still have this weird problem with nodejs not exiting immediately.

BTW, here's the error from that executable:

TypeError: Invalid Version: 0.4.1-nightly.2016.09.09+commit.79867f49.Emscripten.clang
    at new SemVer (/home/cameel/data/working-set/projects/sol/solc-js/node_modules/semver/semver.js:323:11)
    at compare (/home/cameel/data/working-set/projects/sol/solc-js/node_modules/semver/semver.js:614:10)
    at Function.gt (/home/cameel/data/working-set/projects/sol/solc-js/node_modules/semver/semver.js:643:10)
    at setupMethods (/home/cameel/data/working-set/projects/sol/solc-js/wrapper.js:20:27)
    at Object.<anonymous> (/home/cameel/data/working-set/projects/sol/solc-js/index.js:3:18)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)

Looking at it now, I see that it's actually a problem in solc-js, not the executable so it should be fixable. The string it reports is probably just not valid semver. It seems to be the 09 in version. Later executables have no zeros and don't crash.

Copy link
Member

Choose a reason for hiding this comment

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

axic added 5 commits May 17, 2020 16:00
solc-js or emscripten causes some kind of memory leak and node.js runs out of memory
@axic axic force-pushed the reported-version branch from 75d4816 to 84161a8 Compare May 17, 2020 21:46
@chriseth
Copy link
Contributor

Needs rebase.

@axic
Copy link
Member Author

axic commented Mar 12, 2021

@cameel what do you think about this? Since you have been rebuilding binaries. The goal of this was to include the version hardcoded in the binary into the JSON list. Currently (or at the time of this PR) the JSON only contained the normalised version, but certain old compilers do not follow that. Here's a list of discrepancies: https://github.com/ethereum/solc-js/blob/master/translate.js#L3

@cameel
Copy link
Member

cameel commented Mar 12, 2021

I'm not sure if it's actually useful for users but it would definitely be helpful for us to see these versions somewhere just as a sanity check. Putting them on the list does make them and any changes pretty visible.

At times I did need that info myself and I had to gather it manually. I think that seeing them would also make us spot some problems much earlier: #64, ethereum/solidity#7512, ethereum/solidity#10183.

@cameel
Copy link
Member

cameel commented Mar 12, 2021

Here's a list of discrepancies: https://github.com/ethereum/solc-js/blob/master/translate.js#L3

The list is bigger :) Versions listed there look like they're just examples of each format. Actually all emscripten binaries below 0.4.0 use non-standard version format. And 0.4.0 and 0.4.2 also were built with uncommitted changes so their version strings contain .mod. I checked them all at some point - here's the full list of all version strings for emscripten releases and nightlies up to 0.7.3: #64 (comment)

@axic
Copy link
Member Author

axic commented Mar 12, 2021

True, that list was the cases which need parsing. And ones with .mod still parse with semver, so they never showed up as issues. However you mentioning .mod makes me believe this would be actually a very useful feature for sanity check.

@@ -7,6 +7,7 @@ const path = require('path')
const semver = require('semver')
const ethUtil = require('ethereumjs-util')
const swarmhash = require('swarmhash')
const cp = require('child_process')
Copy link
Member

Choose a reason for hiding this comment

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

This is not used that often so I'd use full name for readability:

Suggested change
const cp = require('child_process')
const child_process = require('child_process')

cp reads like the command-line copying utility to me :)

Copy link

Choose a reason for hiding this comment

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

very simple.😉

// NOTE: should be using this, but it leaks memory
// return solc(require(path.join(__dirname, dir, file))).version()
const filename = path.join(__dirname, dir, file)
return cp.execSync(`/usr/bin/env node -e "var solc = require('${filename}'); console.log(solc.cwrap(('_solidity_version' in solc) ? 'solidity_version' : 'version', 'string', [])())"`).toString().trim()
Copy link
Member

Choose a reason for hiding this comment

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

It looks like it's going to be very slow. When I was rewriting bytecode comparison scripts and I tried doing something like this, rerunning node for each compilation was very slow (ethereum/solidity#10165 (comment)). A loop in JS was one or two orders of magnitude faster.

Is it actually a leak or is it just that update itself is pretty bad about controlling its memory usage (ethereum/solidity#9956)? Because for the bytecode comparison I run prepare_report.js which compiles around 5k test cases in a loop, twice and it does not run out of memory in the t-bytecode-compare.yml action.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nodejs is bad at controlling memory. We set the loaded emscripten modules to empty reference, but it still won't be GC'd in time somehow. Perhaps it is not a problem anymore with the wasm binaries?

Copy link
Member

Choose a reason for hiding this comment

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

ok, right. It's about loading the compiler into memory, not running it. Bytecode comparison loads only one version.

Perhaps it is not a problem anymore with the wasm binaries?

We still need to run it on all those old asm.js nightlies so I think this does not help us even if it's fixed for wasm. Unless we just compute versions for asm.js once and cache them.

The script now has an option that makes it not recalculate hashes. It could have a similar one for this. The downside would be that if you replace the binary, CI won't update the versions so it would be safer to somehow combine it with detecting which binaries changed in the PR (which adds a bit of complexity to the whole setup).

const fileContent = readFile(pars.path)
pars.reportedVersion = readBuiltinVersion(pars.path)
Copy link
Member

Choose a reason for hiding this comment

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

One problem with adding it now is that the repo contains binaries for multiple platforms and you won't be able to get versions for all of them in one run. We'd need separate runs for different platforms and then we would have to combine gathered info.

@cameel
Copy link
Member

cameel commented Mar 12, 2021

Maybe we should have an extra check that newly added binaries do not contain .mod in their version string? We could easily do that in t-bytecode-compare.yml which already has logic to detect newly added binaries in new PRs (except for Windows ones). We would just have to limit it to versions above 0.4.2.

@axic
Copy link
Member Author

axic commented Mar 12, 2021

Maybe we should have an extra check that newly added binaries do not contain .mod in their version string?

I think that would make sense in any case.

const fileContent = readFile(pars.path)
pars.reportedVersion = readBuiltinVersion(pars.path)
Copy link
Member

Choose a reason for hiding this comment

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

I think we need an option for skipping this when running locally, just like you can skip hash recalculation.

@cameel cameel requested a review from r0qs November 22, 2022 15:21
@r0qs
Copy link
Member

r0qs commented Dec 1, 2022

@axic Is this still a thing? I didn't understand the motivation for having the reportVersion in list.json.

If still relevant, I can finish that PR and also add the flag to ignore it when running locally, as you suggested here: #21 (comment).

However, I don't think we can improve its performance in the way that it is (it took almost one and half hours on my machine, but the out-of-memory error does not happen anymore).
Maybe it would be better to improve the performance of the whole process, i.e., taking over PR #115 and only after that revisiting this one if it still relevant?

Copy link

@Kurdlove Kurdlove left a comment

Choose a reason for hiding this comment

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

best way for this.

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

Successfully merging this pull request may close these issues.

7 participants