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

Eliminate scratch memory used when generating contexts #557

Merged

Conversation

apoelstra
Copy link
Contributor

Builds on #553

src/group_impl.h Outdated Show resolved Hide resolved
@apoelstra apoelstra force-pushed the 2018-09-no-scratch-memory-in-context-build branch from e1dacce to c4273d0 Compare September 22, 2018 21:53
src/ecmult_impl.h Outdated Show resolved Hide resolved
@apoelstra apoelstra force-pushed the 2018-09-no-scratch-memory-in-context-build branch from c4273d0 to 02fcfff Compare September 22, 2018 21:56
@gmaxwell
Copy link
Contributor

Concept ACK. Are you going to batch that extra inv?

@apoelstra
Copy link
Contributor Author

Yep, done.

@apoelstra
Copy link
Contributor Author

@sipa can you take a look at this?

src/group_impl.h Outdated Show resolved Hide resolved
@apoelstra
Copy link
Contributor Author

Replaced all instances of (size_t)-1) with SIZE_MAX. Confirmed that git grep -n '(size_t)\w*-1' runs clean.

@sipa
Copy link
Contributor

sipa commented Nov 6, 2018

Needs rebase on #553 (not urgent, I'll review soon rebased or not).

@apoelstra apoelstra force-pushed the 2018-09-no-scratch-memory-in-context-build branch from 6aa54d2 to 79c785f Compare November 6, 2018 15:55
@apoelstra
Copy link
Contributor Author

Rebased.

src/group_impl.h Outdated Show resolved Hide resolved
src/group_impl.h Outdated Show resolved Hide resolved
src/ecmult_impl.h Outdated Show resolved Hide resolved
src/ecmult_impl.h Outdated Show resolved Hide resolved
src/ecmult_impl.h Outdated Show resolved Hide resolved
@apoelstra
Copy link
Contributor Author

Addressed all comments.

@apoelstra apoelstra force-pushed the 2018-09-no-scratch-memory-in-context-build branch from 0d05ad0 to df763d2 Compare November 8, 2018 17:46
@sipa
Copy link
Contributor

sipa commented Nov 9, 2018

ACK, please squash the fixes in the respective commits?

It's a really cool trick; I hadn't expected it would be possible to reconstruct everything from Y and Z-ratios, but the formulas work out.

It seems this actually makes secp256k1_context_create(SECP256K1_CONTEXT_VERIFY) around 6% faster for me (when disabling static precomputation...).

@apoelstra apoelstra force-pushed the 2018-09-no-scratch-memory-in-context-build branch from df763d2 to dc583f3 Compare November 9, 2018 00:16
@apoelstra
Copy link
Contributor Author

Squashed.

@apoelstra apoelstra force-pushed the 2018-09-no-scratch-memory-in-context-build branch from dc583f3 to ffd3b34 Compare November 9, 2018 00:21
@peterdettman
Copy link
Contributor

I think it's a bit easier to understand if the z-ratios are stored with the y-values that they'll be used with (instead of off by 1).

Like this: peterdettman@f1ab49c

@apoelstra
Copy link
Contributor Author

I think @peterdettman's commit is great, it does simplify the logic a fair bit.

@apoelstra
Copy link
Contributor Author

Added both of Peter's commits squashed into one, and an addition commit that adds even more comments explaining how the effective-affine isomorphic curve plays into everything.

@peterdettman do you think this is better?

@sipa can you comment on this? We're back to "working backward" in the sense that I do a series of equations starting from abstract coordinates and ending up with values that have variable bindings, but it's all pretty explicit now.

src/ecmult_impl.h Outdated Show resolved Hide resolved
@apoelstra apoelstra force-pushed the 2018-09-no-scratch-memory-in-context-build branch 2 times, most recently from 397770c to c750a91 Compare November 10, 2018 13:59
@apoelstra apoelstra force-pushed the 2018-09-no-scratch-memory-in-context-build branch from c750a91 to b3bf5f9 Compare November 10, 2018 14:01
@sipa
Copy link
Contributor

sipa commented Nov 11, 2018

@apoelstra Looks very readable now; I'll review in detail soon.

@peterdettman
Copy link
Contributor

Yes, better, thanks for improving that.

@gmaxwell
Copy link
Contributor

Nice!

@sipa
Copy link
Contributor

sipa commented Nov 20, 2018

utACK b3bf5f9

@apoelstra
Copy link
Contributor Author

Ready for merge?

@sipa sipa merged commit b3bf5f9 into bitcoin-core:master Nov 26, 2018
sipa added a commit that referenced this pull request Nov 26, 2018
b3bf5f9 ecmult_impl: expand comment to explain how effective affine interacts with everything (Andrew Poelstra)
efa783f Store z-ratios in the 'x' coord they'll recover (Peter Dettman)
ffd3b34 add `secp256k1_ge_set_all_gej_var` test which deals with many infinite points (Andrew Poelstra)
84740ac ecmult_impl: save one fe_inv_var (Andrew Poelstra)
4704527 ecmult_impl: eliminate scratch memory used when generating context (Andrew Poelstra)
7f7a2ed ecmult_gen_impl: eliminate scratch memory used when generating context (Andrew Poelstra)

Pull request description:

  Builds on #553

Tree-SHA512: 6031a601a4a476c1d21fc8db219383e7930434d2f199543c61aca0118412322dd814a0109c385ff1f83d16897170dd0c25051697b0f88f15234b0059b661af41
@apoelstra apoelstra deleted the 2018-09-no-scratch-memory-in-context-build branch November 26, 2018 18:06
@gmaxwell
Copy link
Contributor

Addition/subtraction with constants are effectively free except sometimes in the tightest of loops, the compiler will usually remove them, when it doesn't they usually get hidden by pipelines. This code is run-once code, it should be optimized for clarity and maintainability most and testability much more than for size (within reason) and performance.

gmaxwell added a commit that referenced this pull request May 25, 2019
0522caa Explain caller's obligations for preallocated memory (Tim Ruffing)
238305f Move _preallocated functions to separate header (Tim Ruffing)
695feb6 Export _preallocated functions (Tim Ruffing)
814cc78 Add tests for contexts in preallocated memory (Tim Ruffing)
ba12dd0 Check arguments of _preallocated functions (Tim Ruffing)
5feadde Support cloning a context into preallocated memory (Tim Ruffing)
c4fd5da Switch to a single malloc call (Tim Ruffing)
ef020de Add size constants for preallocated memory (Tim Ruffing)
1bf7c05 Prepare for manual memory management in preallocated memory (Tim Ruffing)

Pull request description:

  @apoelstra

  This builds on #557.

  Manually managing memory is always a pain in the ass in some way. I tried to keep the pain manageable. I'm open to suggestions to make this less ugly or error-prone.

  to do:
   * tests
   * export functions

ACKs for commit 0522ca:

Tree-SHA512: 8ddb5b70219b6f095e780a9812d2387ab2a7f399803ce4101e27da504b479a61ebe08b6380568c7ba6f1e73d7d0b1f58a3c0a66fa0fdec7a64cd0740e156ce38
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.

5 participants