-
Notifications
You must be signed in to change notification settings - Fork 76
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
Incremental TD3: limit side-effected restarting using fuel #629
Merged
Merged
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
05ef196
Add TD3 incremental sided restart fuel for limiting transitivity
sim642 c88f534
Add incremental tests with fuel for going through wrapper function
sim642 efc092b
Add option incremental.restart.sided.fuel-only-global
sim642 a7faf9d
Merge branch 'interactive' into restart-sided-fuel
sim642 93ece0f
Merge branch 'interactive' into restart-sided-fuel
sim642 dee744e
Renumber incremental fuel tests
sim642 99f8b20
Fix incremental fuel tests by adding option incremental.restart.write…
sim642 49cd2ce
Revert fuel change to incremental 13-restart-write/01-mutex-simple
sim642 77b3fca
Add descriptions to fuel options
sim642 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
#include <pthread.h> | ||
#include <stdio.h> | ||
|
||
int myglobal; | ||
pthread_mutex_t mutex1 = PTHREAD_MUTEX_INITIALIZER; | ||
pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; | ||
|
||
void *t_fun(void *arg) { | ||
pthread_mutex_lock(&mutex1); | ||
myglobal=myglobal+1; // RACE! | ||
pthread_mutex_unlock(&mutex1); | ||
return NULL; | ||
} | ||
|
||
void wrap() { | ||
pthread_mutex_lock(&mutex2); | ||
myglobal=myglobal+1; // RACE! | ||
pthread_mutex_unlock(&mutex2); | ||
} | ||
|
||
int main(void) { | ||
pthread_t id; | ||
pthread_create(&id, NULL, t_fun, NULL); | ||
wrap(); | ||
pthread_join (id, NULL); | ||
return 0; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"incremental": { | ||
"restart": { | ||
"sided": { | ||
"enabled": true, | ||
"fuel": 2 | ||
}, | ||
"write-only": false | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
--- tests/incremental/11-restart/14-mutex-simple-wrap2.c | ||
+++ tests/incremental/11-restart/14-mutex-simple-wrap2.c | ||
@@ -7,15 +7,15 @@ pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; | ||
|
||
void *t_fun(void *arg) { | ||
pthread_mutex_lock(&mutex1); | ||
- myglobal=myglobal+1; // RACE! | ||
+ myglobal=myglobal+1; // NORACE | ||
pthread_mutex_unlock(&mutex1); | ||
return NULL; | ||
} | ||
|
||
void wrap() { | ||
- pthread_mutex_lock(&mutex2); | ||
- myglobal=myglobal+1; // RACE! | ||
- pthread_mutex_unlock(&mutex2); | ||
+ pthread_mutex_lock(&mutex1); | ||
+ myglobal=myglobal+1; // NORACE | ||
+ pthread_mutex_unlock(&mutex1); | ||
} | ||
|
||
int main(void) { |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
#include <pthread.h> | ||
#include <stdio.h> | ||
|
||
int myglobal; | ||
pthread_mutex_t mutex1 = PTHREAD_MUTEX_INITIALIZER; | ||
pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; | ||
|
||
void *t_fun(void *arg) { | ||
pthread_mutex_lock(&mutex1); | ||
myglobal=myglobal+1; // RACE! | ||
pthread_mutex_unlock(&mutex1); | ||
return NULL; | ||
} | ||
|
||
void wrap() { | ||
pthread_mutex_lock(&mutex2); | ||
myglobal=myglobal+1; // RACE! | ||
pthread_mutex_unlock(&mutex2); | ||
} | ||
|
||
int main(void) { | ||
pthread_t id; | ||
pthread_create(&id, NULL, t_fun, NULL); | ||
wrap(); | ||
pthread_join (id, NULL); | ||
return 0; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"incremental": { | ||
"restart": { | ||
"sided": { | ||
"enabled": true, | ||
"fuel": 1 | ||
}, | ||
"write-only": false | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
--- tests/incremental/11-restart/15-mutex-simple-wrap1.c | ||
+++ tests/incremental/11-restart/15-mutex-simple-wrap1.c | ||
@@ -7,15 +7,15 @@ pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; | ||
|
||
void *t_fun(void *arg) { | ||
pthread_mutex_lock(&mutex1); | ||
- myglobal=myglobal+1; // RACE! | ||
+ myglobal=myglobal+1; // RACE (not enough fuel) | ||
pthread_mutex_unlock(&mutex1); | ||
return NULL; | ||
} | ||
|
||
void wrap() { | ||
- pthread_mutex_lock(&mutex2); | ||
- myglobal=myglobal+1; // RACE! | ||
- pthread_mutex_unlock(&mutex2); | ||
+ pthread_mutex_lock(&mutex1); | ||
+ myglobal=myglobal+1; // RACE (not enough fuel) | ||
+ pthread_mutex_unlock(&mutex1); | ||
} | ||
|
||
int main(void) { |
27 changes: 27 additions & 0 deletions
27
tests/incremental/11-restart/16-mutex-simple-wrap1-global.c
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
#include <pthread.h> | ||
#include <stdio.h> | ||
|
||
int myglobal; | ||
pthread_mutex_t mutex1 = PTHREAD_MUTEX_INITIALIZER; | ||
pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; | ||
|
||
void *t_fun(void *arg) { | ||
pthread_mutex_lock(&mutex1); | ||
myglobal=myglobal+1; // RACE! | ||
pthread_mutex_unlock(&mutex1); | ||
return NULL; | ||
} | ||
|
||
void wrap() { | ||
pthread_mutex_lock(&mutex2); | ||
myglobal=myglobal+1; // RACE! | ||
pthread_mutex_unlock(&mutex2); | ||
} | ||
|
||
int main(void) { | ||
pthread_t id; | ||
pthread_create(&id, NULL, t_fun, NULL); | ||
wrap(); | ||
pthread_join (id, NULL); | ||
return 0; | ||
} |
12 changes: 12 additions & 0 deletions
12
tests/incremental/11-restart/16-mutex-simple-wrap1-global.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
{ | ||
"incremental": { | ||
"restart": { | ||
"sided": { | ||
"enabled": true, | ||
"fuel": 1, | ||
"fuel-only-global": true | ||
}, | ||
"write-only": false | ||
} | ||
} | ||
} |
22 changes: 22 additions & 0 deletions
22
tests/incremental/11-restart/16-mutex-simple-wrap1-global.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
--- tests/incremental/11-restart/16-mutex-simple-wrap1-global.c | ||
+++ tests/incremental/11-restart/16-mutex-simple-wrap1-global.c | ||
@@ -7,15 +7,15 @@ pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; | ||
|
||
void *t_fun(void *arg) { | ||
pthread_mutex_lock(&mutex1); | ||
- myglobal=myglobal+1; // RACE! | ||
+ myglobal=myglobal+1; // NORACE | ||
pthread_mutex_unlock(&mutex1); | ||
return NULL; | ||
} | ||
|
||
void wrap() { | ||
- pthread_mutex_lock(&mutex2); | ||
- myglobal=myglobal+1; // RACE! | ||
- pthread_mutex_unlock(&mutex2); | ||
+ pthread_mutex_lock(&mutex1); | ||
+ myglobal=myglobal+1; // NORACE | ||
+ pthread_mutex_unlock(&mutex1); | ||
} | ||
|
||
int main(void) { |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
#include <pthread.h> | ||
#include <stdio.h> | ||
|
||
int myglobal; | ||
pthread_mutex_t mutex1 = PTHREAD_MUTEX_INITIALIZER; | ||
pthread_mutex_t mutex2 = PTHREAD_MUTEX_INITIALIZER; | ||
|
||
void *t_fun(void *arg) { | ||
pthread_mutex_lock(&mutex1); | ||
myglobal=myglobal+1; // RACE! | ||
pthread_mutex_unlock(&mutex1); | ||
return NULL; | ||
} | ||
|
||
int main(void) { | ||
pthread_t id; | ||
pthread_create(&id, NULL, t_fun, NULL); | ||
pthread_mutex_lock(&mutex2); | ||
myglobal=myglobal+1; // RACE! | ||
pthread_mutex_unlock(&mutex2); | ||
pthread_join (id, NULL); | ||
return 0; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"incremental": { | ||
"restart": { | ||
"sided": { | ||
"enabled": true, | ||
"fuel": 1 | ||
}, | ||
"write-only": false | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just brought this PR up to date with all the
interactive
branch improvements and noticed that the restarting of write-only unknowns during postprocessing was actually doing some of the restarting that a few of the previously added fuel tests were checking (so the tests didn't pass or didn't check anything meaningful about fuel).Hence I added another option to disable the write-only restarting during postprocessing.
But now I'm very confused about #703 and this because:
only-access
andfuel
things for the general side effect restarting.cc: @michael-schwarz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I am confused. We want to have three possible settings:
It seems to me that we need special handling for the second thing here? What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the sequence of settings:
My point is that at the time, there was no option to disable write-only restarting during postprocessing (it was unconditionally enabled). I only added such option here in this PR later.
This means that configurations 1 and 2 were impossible to achieve on the interactive branch. With general restarting enabled, but limited to
only-access
, and disabling yourdestab-with-side
(or similarly using fuel 1), the write-only unknowns got restarted twice: first during the preprocessing restarting and then also during the postprocessing restarting.So indeed, to get that sequence of changes we need fuel 1
only-access
restarting, but it also means those benchmarks were even more flawed than just the issue withdestab-with-side
ending up with excessive restarting.When writing my previous comment, I think I implicitly assumed there wasn't another flaw and so only a single direct benchmark comparison like 1 → 3 made sense in my head, which is how I also got confused and thought neither
destab-with-side
nor fuel would be necessary.Anyway, I'll go forward with getting fuel merged to interactive as decided last week, but this realization means that the
incremental.restart.write-only
option (added just here) is also crucial for doing the re-benchmarking. I hope this doesn't cause a difference in results (the enabled and accidentally excessivedestab-with-side
only-access
restarting probably dominates the special write-only restarting).