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

Dev/ashish #753

Merged
merged 36 commits into from
Aug 5, 2024
Merged

Dev/ashish #753

merged 36 commits into from
Aug 5, 2024

Conversation

xashisk
Copy link

@xashisk xashisk commented Jul 23, 2024

3 (out of previously 221) bugs remaining.
Code changes:

  1. Made machineListByTypePerThread, machineSetPerThread and their 4 getter and setter functions, and getGlobalMachine() and addGlobalMachine() defined inside the abstract Scheduler.java i.e. local to each Scheduler object.
  2. Add a replayScheduler field in PExplicitGlobal (default value is null). On replaying, we set it to the ReplayScheduler object. Changed PExplicitLogger.isReplaying() function to return true if replayScheduler field in PExplicitGlobal is not null.
  3. Implemented PExplicitGlobal.getScheduler() which first checks isReplaying() or not. If yes, return replayScheduler. If not, get the current thread id, get corresponding localtID, and then return corresponding ExplicitSearchScheduler object. Change all calls to PexplicitGlobal.getMachineSet()/getGlobalMachine/addGlobalMachine to PexplicitGlobal.getScheduler().getMachineSet()/getGlobalMachine/addGlobalMachine
  4. Retained mapping from tID to localtID in PExplicitGlobal, as well as localtID to ExplicitSearchScheduler object.
  5. In class BugFoundException, added a local integer buggyLocalTID which is initialized in both constructors of BugFoundException. Then when a replayscheduler object is created in RunTimeExecutor, we add the following mapping to PExplicitGlobal.tID_to_localtID : [replaying thread ID] -> <local ID of buggy trace, found from e.buggyLocalTID, where e is an object of class BugFoundException>. Is required as many functions in Scheduler() use PExplicitGlobal.tID_to_localtID when replaying thread.

Current buggy benchmarks are:

  1. P/Tst/RegressionTests/Integration/DynamicError/SEM_OneMachine_8
  2. P/Tst/RegressionTests/Feature2Stmts/DynamicError/GotoStmt1
  3. P/Tst/RegressionTests/Feature2Stmts/DynamicError/receive6

aman-goel and others added 30 commits June 13, 2024 18:50
Old schedule/data choice changed to schedule/data SearchUnit

Added new class for schedule/data Choice, which is just a wrapper around PMachineId/PValue<?>

TODO: clean up Schedule with new class structure
[PEx] Make search strategies subclasses thread safe, minor refactoring
Copy link
Contributor

Choose a reason for hiding this comment

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

Please undo changes in this file. It is outside the scope of PExplicit-related code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this file as well.

@aman-goel
Copy link
Contributor

Now that there is a function PExplicitGlobal.getScheduler() to get the scheduler object, why is there PExplicitGlobal.getSchedulers()? That is redundant.

* Mapping from machine type to list of all machine instances
*/
@Getter
private final Map<Integer, Map<Class<? extends PMachine>, List<PMachine>>> machineListByTypePerThread = new ConcurrentHashMap<>(); // This is per thread; so make this map of tiD to same Map
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this per thread? Isn't each Scheduler object already per thread?

* Set of machines
*/
@Getter
private final Map<Integer, SortedSet<PMachine>> machineSetPerThread = new ConcurrentHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. Why is this per thread? Isn't each Scheduler object already per thread?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should directly use PExplicit.getScheduler() throughout this and other files, instead of first getting localTID with PExplicit.getTId_toLocalTID() and then getting the Scheduler object using PExplicit.getSchedulers().get(localTID).

public BugFoundException(String message) {
super(message);
buggyLocalTID = (PExplicitGlobal.getTID_to_localtID()).get(Thread.currentThread().getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. Directly use PExplicitGlobal.getScheduler().

PExplicitLogger.logBugFound(message);
}

public BugFoundException(String message, Throwable cause) {
super(message, cause);
buggyLocalTID = (PExplicitGlobal.getTID_to_localtID()).get(Thread.currentThread().getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. Directly use PExplicitGlobal.getScheduler().

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove all changes outside PExplicitRuntime codebase. They are not in scope here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same. Please remove all changes outside PExplicitRuntime codebase. They are not in scope here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this file. Keep it for your local experimentation only. No need to push upstream.

Copy link
Contributor

@aman-goel aman-goel left a comment

Choose a reason for hiding this comment

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

Shared a bunch of comments to address.

@aman-goel
Copy link
Contributor

Looks like file PExplicitThreadLogger.java line 58 has a hard-coded path. That is making many CI tests fail.

        String filename = "/Users/xashisk/ashish-ws/SyncedForkedRepo/P/output/LogThread" + PExplicitGlobal.getTID_to_localtID().get(Thread.currentThread().getId()) + ".log";  // PIN: Configure output folder with CLI options.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the default number of threads is set to 1. Can we try changing this to a higher number (say 4) and rerun the regression tests (i.e., mvn test).

…s are placed are inside the P folder (else can then change) 3. For nproc 4, added code for killing other threads when bugfoundException is raised
…rror: comment and uncomment L151 and L152, and run for both
@aman-goel aman-goel merged commit 419f335 into p-org:dev/pex_parallel Aug 5, 2024
2 of 5 checks passed
@aman-goel
Copy link
Contributor

Merging on dev branch of parallel PEx (dev/pex_parallel).

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.

3 participants