Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Adjustable stack size for EVM #2483

Merged
merged 5 commits into from Oct 6, 2016
Merged

Adjustable stack size for EVM #2483

merged 5 commits into from Oct 6, 2016

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Oct 5, 2016 •
edited

@NikVolf NikVolf added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Oct 5, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f2186f5 on stack-evm into * on master*.

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 6, 2016
@arkpar arkpar added the B0-patch label Oct 6, 2016
arkpar
arkpar reviewed Oct 6, 2016
View reviewed changes
ethcore/src/executive.rs
/// TODO [todr] We probably need some more sophisticated calculations here (limit on my machine 132)
/// Maybe something like here: `https://github.com/ethereum/libethereum/blob/4db169b8504f2b87f7d5a481819cfb959fc65f6c/libethereum/ExtVM.cpp`
const MAX_VM_DEPTH_FOR_THREAD: usize = 64;
const STACK_SIZE_PER_DEPTH: usize = 256*1024;
Copy link
Collaborator

Choose a reason for hiding this comment

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

256kb per recursion frame? That's too much. Before the change it was 512k / 64 = 8kb
512k is the minimum stack size that we supported

Copy link
Contributor Author

@NikVolf NikVolf Oct 6, 2016 •
edited

Choose a reason for hiding this comment

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

before change it was 2MB/64 = 32kb
i just used 16MB/64 instead, my bad

arkpar
arkpar reviewed Oct 6, 2016
View reviewed changes
util/io/src/worker.rs
/// Stack size
/// Should be modified if it is changed in Rust since it is no way
/// to know or get it
pub static LOCAL_STACK_SIZE: Cell<usize> = Cell::new(::std::env::var("RUST_MIN_STACK").ok().and_then(|s| s.parse().ok()).unwrap_or(2 * 1024 * 1024));
Copy link
Collaborator

Choose a reason for hiding this comment

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

minimum should be 512k, that's the default on OS X

Copy link
Contributor Author

Choose a reason for hiding this comment

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

512k must be default for the main thread only

this expression exactly the same value which rust uses for spawned threads:
https://github.com/rust-lang/rust/blob/master/src/libstd/sys/common/util.rs#L18

arkpar
arkpar reviewed Oct 6, 2016
View reviewed changes
util/io/src/worker.rs

use std::sync::{Condvar as SCondvar, Mutex as SMutex};

const STACK_SIZE: usize = 16*1024*1024;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like with 8kb frame the stack of 8mb should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might be so, but previously we used 32kb
see above

@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 6, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling be2307f on stack-evm into * on master*.

@NikVolf
Copy link
Contributor Author

NikVolf commented Oct 6, 2016

well 16kb was not enough
trying with 24kb

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 841401e on stack-evm into * on master*.

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Oct 6, 2016
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7b956a8 on stack-evm into * on master*.

@NikVolf NikVolf changed the title Flexible stack size for EVM Adjustable stack size for EVM Oct 6, 2016
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 6, 2016
@arkpar arkpar merged commit ac0ae24 into master Oct 6, 2016
arkpar pushed a commit that referenced this pull request Oct 7, 2016
@NikVolf @arkpar
Adjustable stack size for EVM (#2483)
5bb4f2a 
* stack size for io workers & evm threshold

* rust way to remember stack size

* right value

* 24kb size

* some stack reduction
gavofyork pushed a commit that referenced this pull request Oct 7, 2016
@arkpar @gavofyork
Backports into beta (#2512)
bbaf5ed 
* RocksDB version bump

* Preserve cache on reverting the snapshot (#2488)

* Preserve cache on reverting the snapshot

* Renamed merge_with into replace_with

* Renamed and documented snapshotting methods

* Track dirty accounts in the state (#2461)

* State to track dirty accounts

* Removed clone_for_snapshot

* Renaming stuff

* Documentation and other minor fixes

* Replaced MaybeAccount with Option

* Adjustable stack size for EVM (#2483)

* stack size for io workers & evm threshold

* rust way to remember stack size

* right value

* 24kb size

* some stack reduction

* Fixed overflow panic in handshake_panic (#2495)
jacogr added a commit that referenced this pull request Oct 8, 2016
@jacogr
Merge branch 'js' into jr-registry-manage-records
83b6a9c 
* js: (228 commits)
  registration in place
  Backports to master (#2530)
  lookup hash
  ethcore_hashContent call
  single input for commit/filename
  basic githubhint layout
  Handle reorganizations in the state cache (#2490)
  terminate after 30 seconds (#2513)
  allow updates of the secure token
  Using pending block only if not old (#2514)
  Caching optimizations (#2505)
  rework connection display
  basic test for manual token
  Fixed overflow panic in handshake_panic (#2495)
  Trim password from file (#2503)
  Fixing RPC Filter conversion to EthFilter (#2500)
  init token updates take place
  initial token connection - WIP
  Fixing error message for transactions (#2496)
  Adjustable stack size for EVM (#2483)
  ...

# Conflicts:
#	js/src/dapps/registry/Application/application.js
#	js/src/dapps/registry/Container.js
#	js/src/dapps/registry/actions.js
#	js/src/dapps/registry/reducers.js
@gavofyork gavofyork deleted the stack-evm branch November 3, 2016 11:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@arkpar arkpar arkpar left review comments

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@NikVolf @coveralls @arkpar