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

Keep the stack items as an instance of bn.js #159

Merged
merged 4 commits into from Jan 30, 2018

Conversation

Projects
None yet
3 participants
@axic
Member

axic commented Jul 28, 2017

Fixes #38. Depends on #167, #168 and #174.

@axic axic added the in progress label Jul 28, 2017

@axic axic changed the title from Keep the stack items as an instance of bn.js to [WIP] Keep the stack items as an instance of bn.js Jul 28, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Aug 11, 2017

Member

Down to:

# tests 2412
# pass  2323
# fail  89
Member

axic commented Aug 11, 2017

Down to:

# tests 2412
# pass  2323
# fail  89
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 9, 2017

Member

Rebased this locally over #174, will push once that is merged.

Member

axic commented Dec 9, 2017

Rebased this locally over #174, will push once that is merged.

@axic axic changed the title from [WIP] Keep the stack items as an instance of bn.js to Keep the stack items as an instance of bn.js Dec 10, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 10, 2017

Member

@jwasinger I think this should work now too. Rebased and fixed some bugs.

Member

axic commented Dec 10, 2017

@jwasinger I think this should work now too. Rebased and fixed some bugs.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Dec 12, 2017

Member

Fixed returndatasize, returndatacopy, jumpi and log. Push may be broken.

Member

axic commented Dec 12, 2017

Fixed returndatasize, returndatacopy, jumpi and log. Push may be broken.

@jwasinger jwasinger self-assigned this Dec 15, 2017

@jwasinger

This comment has been minimized.

Show comment
Hide comment
@jwasinger

jwasinger Dec 19, 2017

Member

SSTORE is also probably broken here:

https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/opFns.js#L401-L410

{0}.length != {}.length even though they should effectively be treated the same. I tried to fix this but somehow the arguments to SSTORE were not BN (maybe this is because previous pushes (broken?) are not putting BNs on the stack somehow..)

Member

jwasinger commented Dec 19, 2017

SSTORE is also probably broken here:

https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/opFns.js#L401-L410

{0}.length != {}.length even though they should effectively be treated the same. I tried to fix this but somehow the arguments to SSTORE were not BN (maybe this is because previous pushes (broken?) are not putting BNs on the stack somehow..)

@jwasinger jwasinger removed their assignment Dec 19, 2017

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 5, 2018

Member

@jwasinger I think I've added back the lost changes. Fixed DUP also to duplicate the stack item (and not just put a second reference there). It may work?

Member

axic commented Jan 5, 2018

@jwasinger I think I've added back the lost changes. Fixed DUP also to duplicate the stack item (and not just put a second reference there). It may work?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 22, 2018

Member

@holgerd77 do you want to take a look at this? This is the only remaining big internal cleanup PR.

Member

axic commented Jan 22, 2018

@holgerd77 do you want to take a look at this? This is the only remaining big internal cleanup PR.

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 22, 2018

Member

@axic Yes, can do.

Member

holgerd77 commented Jan 22, 2018

@axic Yes, can do.

@holgerd77 holgerd77 self-assigned this Jan 22, 2018

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 25, 2018

Member

Current status:

1..4646
# tests 4646
# pass  3993
# fail  653

(Where do these additional 2000+ tests come from?)

Member

holgerd77 commented Jan 25, 2018

Current status:

1..4646
# tests 4646
# pass  3993
# fail  653

(Where do these additional 2000+ tests come from?)

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 26, 2018

Member

Ok, that was a tiny change, bringing down the failure rate to a level where it was before above:

1..4646
# tests 4646
# pass  4551
# fail  95

Will now look into the remaining tests failing.

Member

holgerd77 commented Jan 26, 2018

Ok, that was a tiny change, bringing down the failure rate to a level where it was before above:

1..4646
# tests 4646
# pass  4551
# fail  95

Will now look into the remaining tests failing.

@jwasinger

This comment has been minimized.

Show comment
Hide comment
@jwasinger

jwasinger Jan 26, 2018

Member

(Where do these additional 2000+ tests come from?)

Probably test cases for Byzantium precompiles

Member

jwasinger commented Jan 26, 2018

(Where do these additional 2000+ tests come from?)

Probably test cases for Byzantium precompiles

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 26, 2018

Member

Two other fixes, this is now down to 0 failures:

1..4646
# tests 4646
# pass  4646

# ok

😄

Let's wait with merging at least until Monday though, maybe someone wants to have another one-step-back view on this.

Member

holgerd77 commented Jan 26, 2018

Two other fixes, this is now down to 0 failures:

1..4646
# tests 4646
# pass  4646

# ok

😄

Let's wait with merging at least until Monday though, maybe someone wants to have another one-step-back view on this.

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 26, 2018

Member

Short speed comparison (on local MacBook Air) running the state tests:

feature/bn-as-stack branch: 4:04 min
master branch: 4:08 min

Hmm, seems this was more for easier reading than for speed though.

Member

holgerd77 commented Jan 26, 2018

Short speed comparison (on local MacBook Air) running the state tests:

feature/bn-as-stack branch: 4:04 min
master branch: 4:08 min

Hmm, seems this was more for easier reading than for speed though.

Show outdated Hide outdated lib/opFns.js Outdated
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 26, 2018

Member

@holgerd77 that optimisation works - do you want to double check it?

Member

axic commented Jan 26, 2018

@holgerd77 that optimisation works - do you want to double check it?

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 26, 2018

Member

Should we do a minor release after this is merged?

@axic: I have to admit I'm not getting your new trick. 😎 😸

Member

holgerd77 commented Jan 26, 2018

Should we do a minor release after this is merged?

@axic: I have to admit I'm not getting your new trick. 😎 😸

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 26, 2018

Member

.andln is bitwise AND, so basically we get the lowest 1 byte (with 0xff) out of BN. But .andln, unlike any other bn.js function, doesn't return a new BN instance, rather a Javascript number.

Sure, we can make a release.

Member

axic commented Jan 26, 2018

.andln is bitwise AND, so basically we get the lowest 1 byte (with 0xff) out of BN. But .andln, unlike any other bn.js function, doesn't return a new BN instance, rather a Javascript number.

Sure, we can make a release.

Show outdated Hide outdated lib/opFns.js Outdated
Show outdated Hide outdated lib/opFns.js Outdated
@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 30, 2018

Member

@holgerd77 are you OK with squashing down the MSTORE8 changes and merging?

Member

axic commented Jan 30, 2018

@holgerd77 are you OK with squashing down the MSTORE8 changes and merging?

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 30, 2018

Member

@axic Is it possible (and did you mean that variant) squashing the MSTORE8 commit into the first one 888ed25 "Keep the stack items as BN.js instances"? Or would you (should I?) just squash the commit into the previous one? (or is there another possible way to proceed here?)

Member

holgerd77 commented Jan 30, 2018

@axic Is it possible (and did you mean that variant) squashing the MSTORE8 commit into the first one 888ed25 "Keep the stack items as BN.js instances"? Or would you (should I?) just squash the commit into the previous one? (or is there another possible way to proceed here?)

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 30, 2018

Member

@holgerd77 I'll do the squashing of that one into the original, but leaving every other fix intact.

Just asking if you are happy with the last solution?

Member

axic commented Jan 30, 2018

@holgerd77 I'll do the squashing of that one into the original, but leaving every other fix intact.

Just asking if you are happy with the last solution?

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 30, 2018

Member

Yes. Just curious how you will do that (squashing into the original, skipping the in-between commits) Not seeing anything for that in interactive rebase command selection. Any trick?

Member

holgerd77 commented Jan 30, 2018

Yes. Just curious how you will do that (squashing into the original, skipping the in-between commits) Not seeing anything for that in interactive rebase command selection. Any trick?

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 30, 2018

Member

Try interactive rebase: git rebase -i HEAD~6 (offers interactive rebasing of the last 6 commits).

Member

axic commented Jan 30, 2018

Try interactive rebase: git rebase -i HEAD~6 (offers interactive rebasing of the last 6 commits).

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 30, 2018

Member

Ah, this note "These lines can be re-ordered;" from the rebase instructions was the missing part for me, didn't know that.

Member

holgerd77 commented Jan 30, 2018

Ah, this note "These lines can be re-ordered;" from the rebase instructions was the missing part for me, didn't know that.

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 30, 2018

Member

@holgerd77 ok to accept?

Member

axic commented Jan 30, 2018

@holgerd77 ok to accept?

@holgerd77

This comment has been minimized.

Show comment
Hide comment
@holgerd77

holgerd77 Jan 30, 2018

Member

Yes (the "Yes" from above was also already answering this. Sorry maybe too hidden. :-)).

Member

holgerd77 commented Jan 30, 2018

Yes (the "Yes" from above was also already answering this. Sorry maybe too hidden. :-)).

@holgerd77

Looks good, can be merged when tests pass.

@axic axic merged commit 0fba8e3 into master Jan 30, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@axic axic removed the in progress label Jan 30, 2018

@axic axic deleted the feature/bn-as-stack branch Jan 30, 2018

@axic

This comment has been minimized.

Show comment
Hide comment
@axic

axic Jan 30, 2018

Member

Hmm, seems this was more for easier reading than for speed though.

I think there should be a more measurable speed increase on running contracts doing arithmetics. See some of @gcolvin's benchmarks for rc5, division and exponentiation.

Member

axic commented Jan 30, 2018

Hmm, seems this was more for easier reading than for speed though.

I think there should be a more measurable speed increase on running contracts doing arithmetics. See some of @gcolvin's benchmarks for rc5, division and exponentiation.

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