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

Benchmark/towers #26

Merged
merged 10 commits into from
Feb 13, 2024
Merged

Benchmark/towers #26

merged 10 commits into from
Feb 13, 2024

Conversation

IR0NSIGHT
Copy link
Owner

fixes #14

Copy link
Contributor

@phischu phischu left a comment

Choose a reason for hiding this comment

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

This can be much simplified if you define your types as

type Disk = Int
type Pile = List[Disk]

In this case deviating from the original is warranted.

//each list represents one smallerDisk.
//TODO type alias, rename as "smallerDisk"
//smallerDisk.next is the smallerDisk below => kellerstack
type Disk = List[Int]
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this to Disks or Pile or Stack. A single disk is an integer (its size).

Copy link
Owner Author

Choose a reason for hiding this comment

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

are we sure we want to deviate from the original?
it would change the semantics quite a lot and probably make it less comparable to the JS implementation of the benchmark.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, do it. The semantics will stay the same, we are merely using types to enforce preconditions.

src/towers.effekt Outdated Show resolved Hide resolved
innerBenchmarkLoop(1){benchmark}{verifyResult}
}

def diskSize(d: Disk): Int = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is head on lists. Use this instead.

var piles: Array[Disk] = emptyArray();
var movesDone: Int = 0;

def pushDisk(piles: Array[Disk], smallerDisk: Disk, pileIdx: Int): Disk = {
Copy link
Contributor

Choose a reason for hiding this comment

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

smallerDisk should be an Int. You could introduce an alias type Disk = Int.

return currentTopDisk
}

def popDiskFrom(pileIdx: Int): Disk = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This one does not abctract over piles, while pushDisk does. It should be consistent.

Copy link
Owner Author

Choose a reason for hiding this comment

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

the signatures of popDiskFrom and pushDisk are taken from the javascript benchmarks.
i would prefer to not change them, to stay as true to the original as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

The signatures currently are:

def pushDisk(piles: Array[Disk], smallerDisk: Disk, pileIdx: Int): Disk
def popDiskFrom(pileIdx: Int): Disk

Where in the original they are:

pushDisk(disk, pile)
popDiskFrom(pile)

So in Effekt they should be either:

def pushDisk(piles: Array[Disk], smallerDisk: Disk, pileIdx: Int): Disk
def popDiskFrom(piles: Array[Disk], pileIdx: Int): Disk

Or they should be:

def pushDisk(smallerDisk: Disk, pileIdx: Int): Disk
def popDiskFrom(pileIdx: Int): Disk

@IR0NSIGHT IR0NSIGHT force-pushed the benchmark/towers branch 2 times, most recently from 8c0eab2 to b5b6f31 Compare January 11, 2024 13:11
rename field
add towers benchmark to runner
fix towers benchmark, used list left commented out debug helper stuff in there in case i ever have to touch it again
@IR0NSIGHT IR0NSIGHT merged commit a18f7ab into master Feb 13, 2024
1 check passed
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.

Towers
2 participants