Static scheduler

[lsk567]

This is a PR for keeping track of the progress on a static scheduler.

[cmnrd]

@ChadliaJerad it looks like several commits from master got rebased and pushed to this branch and are now duplicated. Please use git merge to update a feature branch or rebase the feature branch on top of master (not the other way around). This way, we can keep a clean history.

[ChadliaJerad]

@ChadliaJerad it looks like several commits from master got rebased and pushed to this branch and are now duplicated. Please use git merge to update a future branch or rebase the future branch on top of master (not the other way around). This way, we can keep a clean history.

Yes! I was indeed wondering why are all these commits there.

[lsk567]

Thanks @cmnrd for catching this issue. I just cherry-picked the relevant commits and force-pushed to this branch. The history should be clean now.

[oowekyala]

A couple of comments to improve code quality

[oowekyala]

A suggestion to get rid of the boilerplate: use Kotlin :)

enum class OpCode {
    ADV,
    ADV2,
    BIT
}

// Sealed classes are abstract by default, and the compiler
// knows all their subtypes to allow for exhaustivity checking
// when pattern-matching.
sealed class EvmInstruction(val opcode: OpCode)

// Data classes get a nice toString generated
data class InstructionADV(
    val reactor: ReactorInstance,
    val nextTimeValue: TimeValue
) : EvmInstruction(OpCode.ADV2)

// Since this instruction has no fields it can be a singleton
object InstructionBIT : EvmInstruction(OpCode.BIT)

// ...

This entire package could fit in 100 lines in a single file :)

You can refer to Kotlin classes from Java as if they were regular Java classes. But if you want to stick to Java I would recommend making the supertype sealed (public abstract sealed class Instruction). (doc)

Also I would suggest giving this Instruction class a more specific name like EvmInstruction for clarity

[lsk567]

Thanks for the mini Kotlin tutorial here, @oowekyala ! Yes, Kotlin looks quite interesting. I will try to reimplement the instructions using your solution.

[oowekyala]

I would suggest not extending this class but hiding it inside. As it is you inherit also the offer method which inserts an element, but does not constrain uniqueness as your implementation claims.

[lsk567]

Yes, that makes sense!

[oowekyala]

This class looks like it could be a record

public record Tag(long timestamp, long microstep, boolean forever)
   implements Comparable<Tag> { // ...

This would spare you the fields and ctor declaration, and the equals declaration.

[oowekyala]

Constant patterns should be put into constant fields to avoid recompiling them

[oowekyala]

These should be put in a try-with-resources (TWR). The close method should be in a finally block, which isn't necessary with a TWR.

try (BufferedReader bufferedReader = Files.newBufferedReader(Paths.get(dotFileName))) {
 // your code here
} catch (IOException e) {
  // your handler here
}
// no need to close explicitly

The exception handler doesn't look useful either. I think you should let the IOException propagate to the caller and let the caller handle it in a more sensible way. You can just omit the catch block of your TWR to do that. The boolean return value of this method would not be needed.

[oowekyala]

Actually it occurs to me that this whole int extraction logic could be replaced by using the capture groups of the matcher. Consider rewriting your pattern

"^\\s*(\\d+)\\s*->\\s*(\\d+)"

to remove useless capture groups. Notice also that we use \\s and not \s which is a string escape and not what we want. Then after matcher.find() all you need is to write

int srcNodeId = Integer.parseInt(matcher.group(1));
int sinkNodeId = Integer.parseInt(matcher.group(2));
this.addEdge(srcNodeId, sinkNodeId);

This also makes it clear that what you're giving to your Integer.parseInt is a sequence of digits so you don't need to catch a NumberFormatException, that will never occur.

[lsk567]

Thanks for your detailed review, @oowekyala! I will work on the issues you pointed out and try to make the implementation more robust.

[lhstrh]

@ChadliaJerad it looks like several commits from master got rebased and pushed to this branch and are now duplicated. Please use git merge to update a feature branch or rebase the feature branch on top of master (not the other way around). This way, we can keep a clean history.

Looks like there is a similar problem with duplicate commits in #1695.

[lsk567]

It seems like the Draft status prevents the CI tests from being run. To invoke CI, I will mark the PR as ready for review.