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

Low memory prover #2670

Merged
merged 6 commits into from Nov 6, 2017

Conversation

Projects
None yet
8 participants
@arielgabizon
Contributor

arielgabizon commented Oct 18, 2017

This PR integrates @ebfull 's low memory changes: https://github.com/zcash/zcash/pull/2243/commits
on top of @str4d 's work bringing in libsnark as a subtree
4699d0e

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 19, 2017

Contributor

Nice! I'm going to review the PR later this week, but one initial comment: please rebase this PR to edit the first commit to restore its original commit message (use git rebase -i master, change the first commit from pick to r, change the commit message, and then git push -f to update this PR).

Contributor

str4d commented Oct 19, 2017

Nice! I'm going to review the PR later this week, but one initial comment: please rebase this PR to edit the first commit to restore its original commit message (use git rebase -i master, change the first commit from pick to r, change the commit message, and then git push -f to update this PR).

@str4d str4d added this to the 1.0.13 milestone Oct 19, 2017

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 19, 2017

Contributor

Done.

Contributor

arielgabizon commented Oct 19, 2017

Done.

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Oct 21, 2017

Contributor

The commit message "Delete libsnark tests that we've merged into gtest." does not match the contents of that commit.

Contributor

daira commented Oct 21, 2017

The commit message "Delete libsnark tests that we've merged into gtest." does not match the contents of that commit.

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 21, 2017

Contributor

My mistake. Should be fine now. Though rebase still makes me a little uncomfortable so would be happy if someone verified.

Contributor

arielgabizon commented Oct 21, 2017

My mistake. Should be fine now. Though rebase still makes me a little uncomfortable so would be happy if someone verified.

assert(pk.C_query.domain_size() == qap_wit.num_variables()+2);
assert(pk.H_query.size() == qap_wit.degree()+1);
assert(pk.K_query.size() == qap_wit.num_variables()+4);
#endif

This comment has been minimized.

@daira

daira Oct 22, 2017

Contributor

These debug assertions have been dropped. Is this intentional?

@daira

daira Oct 22, 2017

Contributor

These debug assertions have been dropped. Is this intentional?

This comment has been minimized.

@arielgabizon
@arielgabizon

arielgabizon Oct 24, 2017

Contributor

This comment has been minimized.

@ebfull

ebfull Oct 26, 2017

Contributor

They could be left in.

@ebfull

ebfull Oct 26, 2017

Contributor

They could be left in.

This comment has been minimized.

@arielgabizon

arielgabizon Oct 26, 2017

Contributor

I think this can't be left in cause it needs all the proving key at once.

@arielgabizon

arielgabizon Oct 26, 2017

Contributor

I think this can't be left in cause it needs all the proving key at once.

This comment has been minimized.

@str4d

str4d Oct 31, 2017

Contributor

Most of this could be split across the various functions. The for loop would be harder though, and given that we don't set DEBUG, I'm fine with leaving these out.

@str4d

str4d Oct 31, 2017

Contributor

Most of this could be split across the various functions. The for loop would be harder though, and given that we don't set DEBUG, I'm fine with leaving these out.

Show outdated Hide outdated src/snark/src/zk_proof_systems/ppzksnark/r1cs_ppzksnark/r1cs_ppzksnark.tcc Outdated
Show outdated Hide outdated src/zcash/JoinSplit.cpp Outdated
Show outdated Hide outdated src/zcash/JoinSplit.cpp Outdated
@str4d

Concept ACK and preliminary utACK. I've checked that the refactor appears to be a no-op. Daira's comments need addressing.

Show outdated Hide outdated src/snark/src/zk_proof_systems/ppzksnark/r1cs_ppzksnark/r1cs_ppzksnark.tcc Outdated
assert(pk.C_query.domain_size() == qap_wit.num_variables()+2);
assert(pk.H_query.size() == qap_wit.degree()+1);
assert(pk.K_query.size() == qap_wit.num_variables()+4);
#endif

This comment has been minimized.

@ebfull

ebfull Oct 26, 2017

Contributor

They could be left in.

@ebfull

ebfull Oct 26, 2017

Contributor

They could be left in.

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Oct 26, 2017

Contributor

The math is good, the code and logic is good, I like the API changes, nice and clean.

Just might as well put those DEBUG assertions back even if we're not enabling them ever. (I consider that kind of pre-processor-macro-debug-assertion to be an anti-pattern though, since it doesn't expand templates and create static assertions and things like that.)

Contributor

ebfull commented Oct 26, 2017

The math is good, the code and logic is good, I like the API changes, nice and clean.

Just might as well put those DEBUG assertions back even if we're not enabling them ever. (I consider that kind of pre-processor-macro-debug-assertion to be an anti-pattern though, since it doesn't expand templates and create static assertions and things like that.)

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Oct 27, 2017

Contributor

There might be a memory leak. Tops out with virtual memory at 9GB and actual usage spiking to 7GB.

VIRT      RES
978988  26544  (after ./zcashd) 
2936904 1.471g (after ./zcash-cli zcbenchmark createjoinsplit 1)
4960356 3.080g (repeat...)
6983808 4.689g
9007260 6.298g
9007260 6.462g
9007260 6.462g (repeat... final 6th invocation)

The above was conducted with zcash in regtest mode.

Contributor

bitcartel commented Oct 27, 2017

There might be a memory leak. Tops out with virtual memory at 9GB and actual usage spiking to 7GB.

VIRT      RES
978988  26544  (after ./zcashd) 
2936904 1.471g (after ./zcash-cli zcbenchmark createjoinsplit 1)
4960356 3.080g (repeat...)
6983808 4.689g
9007260 6.298g
9007260 6.462g
9007260 6.462g (repeat... final 6th invocation)

The above was conducted with zcash in regtest mode.

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 27, 2017

Contributor

@bitcartel I wonder if it's related to this comment 929b9db#r120993316

If I understand what @daira said there, it's not a problem?

Contributor

arielgabizon commented Oct 27, 2017

@bitcartel I wonder if it's related to this comment 929b9db#r120993316

If I understand what @daira said there, it's not a problem?

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 27, 2017

Contributor

Btw, wouldn't you get a similar thing with the regular joinsplit before these changes? I remember memory not going down at the end of a joinsplit when checking with htop

Contributor

arielgabizon commented Oct 27, 2017

Btw, wouldn't you get a similar thing with the regular joinsplit before these changes? I remember memory not going down at the end of a joinsplit when checking with htop

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 27, 2017

Contributor

corrected link to Daira's comment #2243 (comment)

Contributor

arielgabizon commented Oct 27, 2017

corrected link to Daira's comment #2243 (comment)

@tromer

This comment has been minimized.

Show comment
Hide comment
@tromer

tromer Oct 27, 2017

Contributor

@arielgabizon This depends on the libc implementation (did the allocator use the heap pool or dedicated pages? if it's the heap pool, how fragmented is it?). This is non-deterministic platform-specific voodoo, and I would not be surprised if "often" the memory remains reserved as far as the OS can see. To be sure it's really freed up, we'd have to take over and do the mmap() by ourselves.

Contributor

tromer commented Oct 27, 2017

@arielgabizon This depends on the libc implementation (did the allocator use the heap pool or dedicated pages? if it's the heap pool, how fragmented is it?). This is non-deterministic platform-specific voodoo, and I would not be surprised if "often" the memory remains reserved as far as the OS can see. To be sure it's really freed up, we'd have to take over and do the mmap() by ourselves.

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 27, 2017

Contributor

@tromer I don't know this stuff, and not sure how you do it yourself; would be happy to fix it if someone explained it to me. @daira said here #2679 that it currently allocates on the stack and we should change that. Though she also said in the comment I linked above that it can wait for a future PR.

Contributor

arielgabizon commented Oct 27, 2017

@tromer I don't know this stuff, and not sure how you do it yourself; would be happy to fix it if someone explained it to me. @daira said here #2679 that it currently allocates on the stack and we should change that. Though she also said in the comment I linked above that it can wait for a future PR.

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 27, 2017

Contributor

Ohh you perhaps were talking about the joinsplit before this code change.

Contributor

arielgabizon commented Oct 27, 2017

Ohh you perhaps were talking about the joinsplit before this code change.

@tromer

This comment has been minimized.

Show comment
Hide comment
@tromer

tromer Oct 27, 2017

Contributor

Yes. @daira is right that it should be on the heap, and that's all that matters for the current PR.

An additional issue, which should not be addressed in the current PR, is that even when using the heap (whether it's the old prover or the new prover), the memory may remain reserved unless you use a special allocator. Fixing that would probably be platform-specific and a bit tricky.

Contributor

tromer commented Oct 27, 2017

Yes. @daira is right that it should be on the heap, and that's all that matters for the current PR.

An additional issue, which should not be addressed in the current PR, is that even when using the heap (whether it's the old prover or the new prover), the memory may remain reserved unless you use a special allocator. Fixing that would probably be platform-specific and a bit tricky.

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 27, 2017

Contributor

@bitcartel you might want to try this version I wrote. https://github.com/arielgabizon/zcash/tree/lowmemmovetoheap
It moves the main variable to the heap, and you can see the more (though not all) memory reclaimed after z_sendmany

Contributor

arielgabizon commented Oct 27, 2017

@bitcartel you might want to try this version I wrote. https://github.com/arielgabizon/zcash/tree/lowmemmovetoheap
It moves the main variable to the heap, and you can see the more (though not all) memory reclaimed after z_sendmany

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Oct 27, 2017

Contributor

@arielg I tried the other branch, same results with zcbenchmark createjoinsplit. Anyway, I don't think this is a blocker, given that this issue already exists and has been documented.

Contributor

bitcartel commented Oct 27, 2017

@arielg I tried the other branch, same results with zcbenchmark createjoinsplit. Anyway, I don't think this is a blocker, given that this issue already exists and has been documented.

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Oct 27, 2017

Contributor

Well glad it's not a blocker. I see more memory released in the other branch when using z_sendmany directly. I wonder if the memory increase is cause the benchmark uses a stack variable.
https://github.com/arielgabizon/zcash/blob/lowmemmovetoheap/src/zcbenchmarks.cpp#L118

Contributor

arielgabizon commented Oct 27, 2017

Well glad it's not a blocker. I see more memory released in the other branch when using z_sendmany directly. I wonder if the memory increase is cause the benchmark uses a stack variable.
https://github.com/arielgabizon/zcash/blob/lowmemmovetoheap/src/zcbenchmarks.cpp#L118

@daira daira dismissed their stale review Oct 28, 2017

Keeping the DEBUG assertions is not a blocker if it's too difficult. Allocating on the stack is also not a blocker.

@daira daira added this to 1.0.13: Performance Improvements in Release planning Oct 30, 2017

@str4d

utACK with fixes

assert(pk.C_query.domain_size() == qap_wit.num_variables()+2);
assert(pk.H_query.size() == qap_wit.degree()+1);
assert(pk.K_query.size() == qap_wit.num_variables()+4);
#endif

This comment has been minimized.

@str4d

str4d Oct 31, 2017

Contributor

Most of this could be split across the various functions. The for loop would be harder though, and given that we don't set DEBUG, I'm fine with leaving these out.

@str4d

str4d Oct 31, 2017

Contributor

Most of this could be split across the various functions. The for loop would be harder though, and given that we don't set DEBUG, I'm fine with leaving these out.

Show outdated Hide outdated src/snark/src/zk_proof_systems/ppzksnark/r1cs_ppzksnark/r1cs_ppzksnark.tcc Outdated
Show outdated Hide outdated src/snark/src/zk_proof_systems/ppzksnark/r1cs_ppzksnark/r1cs_ppzksnark.tcc Outdated
Show outdated Hide outdated src/snark/src/zk_proof_systems/ppzksnark/r1cs_ppzksnark/r1cs_ppzksnark.tcc Outdated

zkbot added a commit that referenced this pull request Nov 2, 2017

Auto merge of #2670 - arielgabizon:lowmemprover, r=str4d
Low memory prover

This PR integrates @ebfull 's low memory changes:  https://github.com/zcash/zcash/pull/2243/commits
on top of @str4d 's work bringing in libsnark as a subtree
4699d0e
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 2, 2017

Contributor

💔 Test failed - pr-merge

Contributor

zkbot commented Nov 2, 2017

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 2, 2017

Contributor

One of the Boost tests is failing. The error message:

unknown location(0): fatal error: in "rpc_wallet_tests/rpc_z_sendmany_internals": memory access violation at address: 0x00000848: no mapping at fault address
test/rpc_wallet_tests.cpp(1171): last checkpoint
Contributor

str4d commented Nov 2, 2017

One of the Boost tests is failing. The error message:

unknown location(0): fatal error: in "rpc_wallet_tests/rpc_z_sendmany_internals": memory access violation at address: 0x00000848: no mapping at fault address
test/rpc_wallet_tests.cpp(1171): last checkpoint
@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Nov 2, 2017

Contributor

Ugh, address looks like an offset from NULL.

Contributor

daira commented Nov 2, 2017

Ugh, address looks like an offset from NULL.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Nov 4, 2017

Contributor

Reproducible. test/test_bitcoin -t rpc_wallet_tests fails in this branch, but passes fine on master branch.

Contributor

bitcartel commented Nov 4, 2017

Reproducible. test/test_bitcoin -t rpc_wallet_tests fails in this branch, but passes fine on master branch.

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Nov 4, 2017

Contributor

Looks like that test fails only with the last commit that puts the ifstream object on the head, which increases running time by 0.5 seconds according to @str4d , maybe just abort that commit for now?

Contributor

arielgabizon commented Nov 4, 2017

Looks like that test fails only with the last commit that puts the ifstream object on the head, which increases running time by 0.5 seconds according to @str4d , maybe just abort that commit for now?

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 5, 2017

Contributor

Force-pushed to remove 2f598a5 from this PR.

@zkbot try

Contributor

str4d commented Nov 5, 2017

Force-pushed to remove 2f598a5 from this PR.

@zkbot try

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 5, 2017

Contributor

⌛️ Trying commit 4305a56 with merge 94e1902...

Contributor

zkbot commented Nov 5, 2017

⌛️ Trying commit 4305a56 with merge 94e1902...

zkbot added a commit that referenced this pull request Nov 5, 2017

Auto merge of #2670 - arielgabizon:lowmemprover, r=<try>
Low memory prover

This PR integrates @ebfull 's low memory changes:  https://github.com/zcash/zcash/pull/2243/commits
on top of @str4d 's work bringing in libsnark as a subtree
4699d0e
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 5, 2017

Contributor

Still seeing that failure:

unknown location(0): fatal error: in "rpc_wallet_tests/rpc_z_sendmany_internals": memory access violation at address: 0x00000270: no mapping at fault address
test/rpc_wallet_tests.cpp(1171): last checkpoint
*** 1 failure is detected in the test module "Bitcoin Test Suite"
test_bitcoin: /home/zcbbworker/latent-0/debian8/build/depends/x86_64-unknown-linux-gnu/share/../include/boost/thread/pthread/condition_variable_fwd.hpp:102: boost::condition_variable::~condition_variable(): Assertion `!ret' failed.
Contributor

str4d commented Nov 5, 2017

Still seeing that failure:

unknown location(0): fatal error: in "rpc_wallet_tests/rpc_z_sendmany_internals": memory access violation at address: 0x00000270: no mapping at fault address
test/rpc_wallet_tests.cpp(1171): last checkpoint
*** 1 failure is detected in the test module "Bitcoin Test Suite"
test_bitcoin: /home/zcbbworker/latent-0/debian8/build/depends/x86_64-unknown-linux-gnu/share/../include/boost/thread/pthread/condition_variable_fwd.hpp:102: boost::condition_variable::~condition_variable(): Assertion `!ret' failed.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 5, 2017

Contributor

💔 Test failed - pr-try

Contributor

zkbot commented Nov 5, 2017

💔 Test failed - pr-try

BasicTestingSetup::BasicTestingSetup()
{
assert(init_and_check_sodium() != -1);
ECC_Start();
pzcashParams = ZCJoinSplit::Unopened();

This comment has been minimized.

@str4d

str4d Nov 5, 2017

Contributor

This is the cause of the crash. Previously TestingSetup had access to pzcashParams because it inherits from BasicTestingSetup. With this change, it doesn't have that defined any more, leading to a null pointer error in rpc_wallet_tests (which uses TestingSetup). I think the best solution is to have TestingSetup inherit from JoinSplitTestingSetup.

@str4d

str4d Nov 5, 2017

Contributor

This is the cause of the crash. Previously TestingSetup had access to pzcashParams because it inherits from BasicTestingSetup. With this change, it doesn't have that defined any more, leading to a null pointer error in rpc_wallet_tests (which uses TestingSetup). I think the best solution is to have TestingSetup inherit from JoinSplitTestingSetup.

This comment has been minimized.

@str4d

str4d Nov 5, 2017

Contributor

Oh, and if we do this, the test that was triggering this bug then fails:

test/rpc_wallet_tests.cpp(1377): error: in "rpc_wallet_tests/rpc_z_shieldcoinbase_internals": check string(e.what()).find("JoinSplit verifying key not loaded")!= string::npos has failed
@str4d

str4d Nov 5, 2017

Contributor

Oh, and if we do this, the test that was triggering this bug then fails:

test/rpc_wallet_tests.cpp(1377): error: in "rpc_wallet_tests/rpc_z_shieldcoinbase_internals": check string(e.what()).find("JoinSplit verifying key not loaded")!= string::npos has failed
@@ -154,10 +110,6 @@ class JoinSplitCircuit : public JoinSplit<NumInputs, NumOutputs> {
uint64_t vpub_new,
const uint256& rt
) {
if (!vk || !vk_precomp) {
throw std::runtime_error("JoinSplit verifying key not loaded");

This comment has been minimized.

@str4d

str4d Nov 5, 2017

Contributor

I'm going to just alter the test to check for the error we actually get now, given that this error has been removed.

@str4d

str4d Nov 5, 2017

Contributor

I'm going to just alter the test to check for the error we actually get now, given that this error has been removed.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Nov 5, 2017

Contributor

@zkbot try

Contributor

str4d commented Nov 5, 2017

@zkbot try

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 5, 2017

Contributor

⌛️ Trying commit bef1b5c with merge 9e03db9...

Contributor

zkbot commented Nov 5, 2017

⌛️ Trying commit bef1b5c with merge 9e03db9...

zkbot added a commit that referenced this pull request Nov 5, 2017

Auto merge of #2670 - arielgabizon:lowmemprover, r=<try>
Low memory prover

This PR integrates @ebfull 's low memory changes:  https://github.com/zcash/zcash/pull/2243/commits
on top of @str4d 's work bringing in libsnark as a subtree
4699d0e
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 5, 2017

Contributor

☀️ Test successful - pr-try
State: approved= try=True

Contributor

zkbot commented Nov 5, 2017

☀️ Test successful - pr-try
State: approved= try=True

@daira

daira approved these changes Nov 6, 2017

ACK bef1b5c

@daira

This comment has been minimized.

Show comment
Hide comment
@daira

daira Nov 6, 2017

Contributor

@zkbot r+

Contributor

daira commented Nov 6, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 6, 2017

Contributor

📌 Commit bef1b5c has been approved by daira

Contributor

zkbot commented Nov 6, 2017

📌 Commit bef1b5c has been approved by daira

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 6, 2017

Contributor

⌛️ Testing commit bef1b5c with merge 6f9f09d...

Contributor

zkbot commented Nov 6, 2017

⌛️ Testing commit bef1b5c with merge 6f9f09d...

zkbot added a commit that referenced this pull request Nov 6, 2017

Auto merge of #2670 - arielgabizon:lowmemprover, r=daira
Low memory prover

This PR integrates @ebfull 's low memory changes:  https://github.com/zcash/zcash/pull/2243/commits
on top of @str4d 's work bringing in libsnark as a subtree
4699d0e
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Nov 6, 2017

Contributor

☀️ Test successful - pr-merge
Approved by: daira
Pushing 6f9f09d to master...

Contributor

zkbot commented Nov 6, 2017

☀️ Test successful - pr-merge
Approved by: daira
Pushing 6f9f09d to master...

@zkbot zkbot merged commit bef1b5c into zcash:master Nov 6, 2017

1 check passed

homu Test successful
Details
@karel-3d

This comment has been minimized.

Show comment
Hide comment
@karel-3d

karel-3d Nov 7, 2017

Thanks for merging! I will be able to run actual release soon :)

karel-3d commented Nov 7, 2017

Thanks for merging! I will be able to run actual release soon :)

@arielgabizon

This comment has been minimized.

Show comment
Hide comment
@arielgabizon

arielgabizon Dec 1, 2017

Contributor

Interestingly, the memory peak now and previously, is not during proof generation but during creating the joinsplit gadget https://github.com/zcash/zcash/blob/master/src/zcash/JoinSplit.cpp#L276.
@madars and other libsnark authors might want to know about this.

Contributor

arielgabizon commented Dec 1, 2017

Interestingly, the memory peak now and previously, is not during proof generation but during creating the joinsplit gadget https://github.com/zcash/zcash/blob/master/src/zcash/JoinSplit.cpp#L276.
@madars and other libsnark authors might want to know about this.

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