Cpu table optmization #21

ear7h · 2018-11-23T23:27:22Z

Implemented a static array for storing operations where the functions are indexed by their opcode. Also removed byte casts from the cbInstruction function as the compiler will already remove the bounds checks from the array. At least based on the assembly output from a similar function:
https://godbolt.org/z/t7uSt1

…or changes to the cb_table function which unecessarily converted indexes to bytes

codecov · 2018-11-23T23:40:06Z

Codecov Report

Merging #21 into master will increase coverage by 4.04%.
The diff coverage is 99.67%.

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   65.07%   69.11%   +4.04%     
==========================================
  Files          20       20              
  Lines        2265     2461     +196     
==========================================
+ Hits         1474     1701     +227     
+ Misses        762      730      -32     
- Partials       29       30       +1

Impacted Files	Coverage Δ
pkg/gb/gameboy.go	`80.89% <100%> (+11.86%)`	⬆️
pkg/gb/instructions_cb.go	`100% <100%> (ø)`	⬆️
pkg/gb/instructions.go	`99.66% <99.65%> (+0.11%)`	⬆️
pkg/gbio/iopixel/pixels.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9f78a9...44458e2. Read the comment docs.

Humpheh · 2018-11-25T11:30:53Z

This is great, thanks @ear7h - I'm going to have a review of it soon, but I thought it would be good to benchmark it first just to check.

I added in the new instruction table alongside the old one and then ran this benchmark to run some random instructions:

package gb

import (
	"math/rand"
	"testing"
	"time"

	"github.com/Humpheh/goboy/pkg/cart"
)

func randomBytes(num int) (out []byte) {
	rand.Seed(time.Now().Unix())
	for i := 0; i < num; i++ {
		out = append(out, byte(rand.Intn(0x100)))
	}
	out[0x147] = 0
	return
}

var randomRom = randomBytes(0x10000)

func testGB() *Gameboy {
	gameboy := Gameboy{}
	gameboy.mainInst = gameboy.mainInstructions()
	gameboy.setup()
	gameboy.Memory.Cart = cart.NewCart(randomRom, "")
	return &gameboy
}

func BenchmarkGameboy_ExecuteNextOpcode(b *testing.B) {
	gameboy := testGB()
	for n := 0; n < b.N; n++ {
		gameboy.ExecuteOpcode(byte(rand.Intn(0x100)))
	}
}

func BenchmarkGameboy_ExecuteNextOpcode2(b *testing.B) {
	gameboy := testGB()
	for n := 0; n < b.N; n++ {
		gameboy.mainInst[byte(rand.Intn(0x100))]()
	}
}

The results were:

goos: darwin
goarch: amd64
pkg: github.com/Humpheh/goboy/pkg/gb

BenchmarkGameboy_ExecuteNextOpcode-4    	10000000	       117 ns/op
BenchmarkGameboy_ExecuteNextOpcode2-4   	20000000	        90.8 ns/op

So the new version is definitely faster!

Humpheh

Looks great, thanks for doing this. Just some code/comment style points :)

Humpheh · 2018-11-25T11:44:18Z

pkg/gb/gameboy.go

 	InputMask byte

+	mainInst [0x100]func()


Could you put the mainInst above the cbInst so that they are grouped together?

Humpheh · 2018-11-25T11:44:33Z

pkg/gb/gameboy.go

@@ -320,6 +322,8 @@ func (gb *Gameboy) setup() error {

 	gb.cbInst = gb.cbInstructions()

+	gb.mainInst = gb.mainInstructions()


Same here, could you put this line above the gb.cbInst line?

Humpheh · 2018-11-25T11:46:19Z

pkg/gb/instructions.go

-		log.Printf("Unimplemented opcode: %#2x", opcode)
-		WaitForInput()
-	}
+	gb.mainInst[opcode]()


I would prefer is the contents of mainInstructions were in this file rather than a separate one. Also it looks like ExecuteOpcode is only called in one place, so it may be better just to replace that line with gb.mainInst[opcode]() and remove this function.

Humpheh · 2018-11-25T11:47:09Z

pkg/gb/instructions_table.go

+		0x06: func() {
+			// LD B, n
+			gb.CPU.BC.SetHi(gb.popPC())
+


Could you remove these blank lines before the end of each function?

Humpheh · 2018-11-25T11:47:52Z

pkg/gb/instructions_table.go

+			val := gb.Memory.ReadHighRam(0xFF00 + uint16(gb.popPC()))
+			gb.CPU.AF.SetHi(val)
+
+			// ========== 16-Bit Loads ===========


This comment is in the wrong place now and should be outside the function.

Humpheh · 2018-11-25T11:48:05Z

pkg/gb/instructions_table.go

+			// POP HL
+			gb.CPU.HL.Set(gb.popStack())
+
+			// ========== 8-Bit ALU ===========


Same with this one.

Humpheh · 2018-11-25T11:48:38Z

pkg/gb/instructions_table.go

+			addr := gb.CPU.HL.HiLo()
+			gb.instDec(func(val byte) { gb.Memory.Write(addr, val) }, gb.Memory.Read(addr))
+
+			// ========== 16-Bit ALU ===========


Another misplaced comment.

Humpheh · 2018-11-25T11:49:20Z

pkg/gb/instructions_table.go

+			gb.instDec16(gb.CPU.SP.Set, gb.CPU.SP.HiLo())
+
+		},
+		0x27: func() { /*


This comment seems to have formatted badly. Could you replace the /* ... */ with just // lines? I think that makes it more consistent with the rest of the code base.

Humpheh · 2018-11-25T11:49:30Z

pkg/gb/instructions_table.go

+			gb.halted = true
+
+		},
+		0x10: func() { //gb.halted = true


This comment can be moved below the // STOP comment

… instructions.go, and minor style changes

Humpheh

This is great, thanks! :)

ear7h added 2 commits November 23, 2018 15:07

changed instruction switch satatement into a function table, some min…

e2690d3

…or changes to the cb_table function which unecessarily converted indexes to bytes

variable scope bug fix for densifying the main instruction table

f936b66

Humpheh requested changes Nov 25, 2018

View reviewed changes

Humpheh mentioned this pull request Nov 25, 2018

Speed up CPU #20

Closed

fixed comments, removed ExecuteOpcode method, moved table creation to…

44458e2

… instructions.go, and minor style changes

Humpheh approved these changes Nov 27, 2018

View reviewed changes

Humpheh merged commit f876370 into Humpheh:master Nov 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cpu table optmization #21

Cpu table optmization #21

ear7h commented Nov 23, 2018

codecov bot commented Nov 23, 2018 •

edited

Loading

Humpheh commented Nov 25, 2018

Humpheh left a comment

Humpheh Nov 25, 2018

Humpheh Nov 25, 2018

Humpheh Nov 25, 2018

Humpheh Nov 25, 2018

Humpheh Nov 25, 2018

Humpheh Nov 25, 2018

Humpheh Nov 25, 2018

Humpheh Nov 25, 2018

Humpheh Nov 25, 2018

Humpheh left a comment

		@@ -320,6 +322,8 @@ func (gb *Gameboy) setup() error {

		gb.cbInst = gb.cbInstructions()

		gb.mainInst = gb.mainInstructions()

Cpu table optmization #21

Cpu table optmization #21

Conversation

ear7h commented Nov 23, 2018

codecov bot commented Nov 23, 2018 • edited Loading

Codecov Report

Humpheh commented Nov 25, 2018

Humpheh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Humpheh left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 23, 2018 •

edited

Loading