Skip to content

changes to thread code (directly use boost::thread) #2553

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.

Sign up for GitHub

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

Jump to bottom
Merged
jgarzik merged 1 commit into from
Jun 10, 2013
Merged

changes to thread code (directly use boost::thread) #2553

jgarzik merged 1 commit into from
Jun 10, 2013

Conversation

@Diapolo
Copy link

@Diapolo Diapolo commented Apr 23, 2013

  • removes our NewThread() function an replaces these calls with
    boost::thread with our TraceThread template
  • make ThreadCleanWalletPassphrase() use an int64 as parameter
  • remove ExitThread() function
  • fix THREAD_PRIORITY_ABOVE_NORMAL for non Windows OSes

@Diapolo
Copy link
Author

Diapolo commented Apr 23, 2013

@BitcoinPullTester Error seems unrelated to my changes!

Exception in thread "main" java.lang.reflect.InvocationTargetException
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:616)
    at org.eclipse.jdt.internal.jarinjarloader.JarRsrcLoader.main(JarRsrcLoader.java:58)
Caused by: java.lang.OutOfMemoryError: Java heap space
    at com.google.bitcoin.core.FullBlockTestGenerator.getBlocksToTest(FullBlockTestGenerator.java:1335)
    at com.google.bitcoin.core.BitcoindComparisonTool.(BitcoindComparisonTool.java:77)
    at com.google.bitcoin.core.BitcoindComparisonTool.main(BitcoindComparisonTool.java:50)
    ... 5 more

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Apr 23, 2013

Oops, false-positive, sorry, retesting...
also, tagging @BitcoinPullTester doesnt email me, please tag @TheBlueMatt instead

gavinandresen
gavinandresen reviewed Apr 24, 2013
View reviewed changes
src/rpcwallet.cpp Outdated
Copy link
Contributor

@gavinandresen gavinandresen Apr 24, 2013

Choose a reason for hiding this comment

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

cast isn't necessary, pnSleepTime is already an int64*

Copy link
Contributor

@gavinandresen gavinandresen Apr 24, 2013

Choose a reason for hiding this comment

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

Also, the code would be simpler if an int64 was passed instead of an int64* -- the only reason a pointer was passed before was because NewThread was dumb and could only pass one pointer.

Copy link
Author

@Diapolo Diapolo Apr 24, 2013

Choose a reason for hiding this comment

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

I updated ThreadCleanWalletPassphrase() to use an int64, can you have another look?

@BitcoinPullTester
Copy link

BitcoinPullTester commented Apr 26, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7df7cf51daf0ac9b491a356222f7217f296b0ace for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@Diapolo
Copy link
Author

Diapolo commented Apr 27, 2013

Any further comments on this?

@BitcoinPullTester
Copy link

BitcoinPullTester commented Apr 30, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/7796fe9da178a92108682f05e9ac037012326cd9 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

sipa
sipa reviewed Apr 30, 2013
View reviewed changes
src/rpcwallet.cpp Outdated
Copy link
Member

@sipa sipa Apr 30, 2013

Choose a reason for hiding this comment

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

nit: remove the p in the name (I don't care much for the type-prefixes in variable names, but when you use them, they should be correct).

@Diapolo
Copy link
Author

Diapolo commented May 1, 2013

@sipa Fixed your nit ;) and you were absolutely right.

@BitcoinPullTester
Copy link

BitcoinPullTester commented May 1, 2013

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/1f52db4fdd3641e2c0ebbc2355ccc27547eaded7 for test log.

This pull does not merge cleanly onto current master
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@Diapolo
Copy link
Author

Diapolo commented May 1, 2013

fatal: '1f52db4fdd3641e2c0ebbc2355ccc27547eaded7' does not point to a commit I updated this 2 times in a row, guess pulltester was too slow ^^.

@BitcoinPullTester
Copy link

BitcoinPullTester commented May 1, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f810bac189c3f12de043026257adda7e76f27adf for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@sipa
Copy link
Member

sipa commented May 1, 2013

ACK

@gavinandresen
Copy link
Contributor

gavinandresen commented May 7, 2013

The wallet passphrase bits of this are unnecessary after #2625

@Diapolo
Copy link
Author

Diapolo commented May 7, 2013

@gavinandresen I will update after your patch was merged or you just merge this an rebase your pull :). That way the other changes in this pull can get in now.

As #2625 seems a little controversial, what is your oppinion on this now Gavin?

@laanwj
Copy link
Member

laanwj commented May 30, 2013

Needs rebase as #2625 was merged

@BitcoinPullTester
Copy link

BitcoinPullTester commented Jun 2, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/0ab7a2d0f0adb7020b0a75fb3a2426f29e61b943 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

changes to thread code (directly use boost::thread)
53e7113 
- removes our NewThread() function an replaces remaining calls with
  boost::thread with our TraceThread template
- remove ExitThread() function
- fix THREAD_PRIORITY_ABOVE_NORMAL for non Windows OSes
jgarzik pushed a commit that referenced this pull request Jun 10, 2013
Merge pull request #2553 from Diapolo/threads
d1020b7 
changes to thread code (directly use boost::thread)
@jgarzik jgarzik merged commit d1020b7 into bitcoin:master Jun 10, 2013
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Diapolo @TheBlueMatt @BitcoinPullTester @sipa @gavinandresen @laanwj @jgarzik