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

build.rs: Add feature 'lowmemory' to reduce memory usage #140

Merged
merged 2 commits into from
Aug 12, 2019
Merged

build.rs: Add feature 'lowmemory' to reduce memory usage #140

merged 2 commits into from
Aug 12, 2019

Conversation

laanwj
Copy link
Contributor

@laanwj laanwj commented Aug 7, 2019

Currently, this only set ECMULT_WINDOW_SIZE to 4 instead of 15.

Fixes #139.

@elichai
Copy link
Member

elichai commented Aug 7, 2019

ACK 09037fc

@apoelstra
Copy link
Member

If you add a commit bumping the version to 0.15.1 (and setting the date in CHANGELOG.md) I can merge and publish today

@laanwj
Copy link
Contributor Author

laanwj commented Aug 7, 2019

Whoops, I didn't realize that the version is already at 0.15.1, https://github.com/rust-bitcoin/rust-secp256k1/blob/master/Cargo.toml#L4
So I think I should bump it to 0.15.2 and change the CHANGELOG.md entry to that?

@real-or-random
Copy link
Collaborator

Do we think 3 is good is that "too low" to be reasonable?

@apoelstra
Copy link
Member

Oh, yeah, 0.15.1 just had a docs.rs fix.

@real-or-random for embedded applications where speed is nonessential I think we want "as low as possible".

@laanwj
Copy link
Contributor Author

laanwj commented Aug 7, 2019

@real-or-random for embedded applications where speed is nonessential I think we want "as low as possible".

3 seems to be fine for one-shot purposes such as verifying firmware signatures. The speed-up is mostly interesting in applications that verify large numbers of signatures.

I once did some benchmarks here: bitcoin-core/secp256k1#614 (comment)

So from 3 to 15 is at most a 18% speed-up, where going to 6 already accounts for about half of that. Even with the lowest setting I'd say it's fast enough for boot-loader use of verifying one or a few signatures.

@laanwj
Copy link
Contributor Author

laanwj commented Aug 8, 2019

added version bump to 0.15.2 and updated the changelog accordingly !

@real-or-random
Copy link
Collaborator

@real-or-random for embedded applications where speed is nonessential I think we want "as low as possible".

If you want as low as possible, that's "2" and not "3".

@laanwj Can you update this?

(I restarted Travis, which had some hiccup).

@apoelstra
Copy link
Member

Travis is still having problems :/

@laanwj
Copy link
Contributor Author

laanwj commented Aug 8, 2019

If you want as low as possible, that's "2" and not "3".
@laanwj Can you update this?

That would be 340 to 170 bytes? don't know if that's worth it. secp256k1 itself is 70k+ compiled.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

I'm not sure either; that's why I started the discussion about a reasonable low value. I think 3 is good, I 'd even pick 4.

build.rs Outdated

if cfg!(feature = "lowmemory") {
base_config.define("ECMULT_WINDOW_SIZE", Some("3")); // This is the lowest accepted value by configure
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want anything > 2, then we shouldn't claim that this is the lowest accepted value.

@laanwj
Copy link
Contributor Author

laanwj commented Aug 12, 2019

OK, I'll set it to 4 and remove that comment.

Currently, this only set `ECMULT_WINDOW_SIZE` to 4 instead of 15.

Fixes #139.

fixup
And fill in release date for 0.15.2.
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

utACK

@apoelstra apoelstra merged commit 288cc1e into rust-bitcoin:master Aug 12, 2019
@apoelstra
Copy link
Member

Published 0.15.2

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.

Need a way to override ECMULT_WINDOW_SIZE externally
4 participants