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

Opbinaryop and other minor optimizations #157

Merged
merged 9 commits into from
Mar 22, 2019
Merged

Opbinaryop and other minor optimizations #157

merged 9 commits into from
Mar 22, 2019

Conversation

geseq
Copy link
Collaborator

@geseq geseq commented Mar 21, 2019

Nothing like the last optimisation but but it does add up to around 4% improvement. I've done some micro optimizations on the v.ip increments. While individually they don't amount to much, they do seem to add up on a fib(35) and likely on any long running script.

OLD:


fibonacci(35)

Result: 9227465
Go: 45.674312ms
Parser: 32.976µs
Compile: 104.373µs
VM: 2.597270621s

fibonacci(35) (tail-call #1)

Result: 9227465
Go: 62.044356ms
Parser: 20.597µs
Compile: 71.31µs
VM: 2.692990659s

fibonacci(35) (tail-call #2)

Result: 9227465
Go: 447ns
Parser: 37.322µs
Compile: 69.521µs
VM: 13.112µs

NEW:


fibonacci(35)

Result: 9227465
Go: 47.159815ms
Parser: 34.767µs
Compile: 115.81µs
VM: 2.44406141s

fibonacci(35) (tail-call #1)

Result: 9227465
Go: 62.935803ms
Parser: 19.642µs
Compile: 64.569µs
VM: 2.52746129s

fibonacci(35) (tail-call #2)

Result: 9227465
Go: 251ns
Parser: 17.535µs
Compile: 49.706µs
VM: 14.56µs

@geseq geseq requested a review from d5 March 21, 2019 15:29
@d5
Copy link
Owner

d5 commented Mar 21, 2019

Using opcodes as a parameter to another opcode doesn't feel right. Can we just add 1-byte operand to OpBinaryOp (for operator token) and delete all other binary opcodes (OpAdd, OpSub, ...)?

@geseq
Copy link
Collaborator Author

geseq commented Mar 21, 2019

Using opcodes as a parameter to another opcode doesn't feel right. Can we just add 1-byte operand to OpBinaryOp (for operator token) and delete all other binary opcodes (OpAdd, OpSub, ...)?

I did try that originally but for some reason that I can't remember now I changed to this. Let me try it again and see how it goes.

@d5
Copy link
Owner

d5 commented Mar 21, 2019

Also realized that we were failing to set v.errOffset properly.

filePos := v.fileSet.Position(v.curFrame.fn.SourceMap[v.ip-v.errOffset]) // <- this will not work

We can probably find a better way to handle this. I will think about this a little bit today.

runtime/vm.go Outdated
for v.ip < v.curIPLimit && (atomic.LoadInt64(&v.aborting) == 0) {
defer func() {
if r := recover(); r != nil {
if v.ip < v.curIPLimit {
Copy link
Owner

Choose a reason for hiding this comment

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

If we keep this panic-recover approach, we can replace this to if v.ip < len(v.curInsts)-1 and remove v.curIPLimit entirely,

@d5
Copy link
Owner

d5 commented Mar 21, 2019

One solution to IP offset is to add the following method to CompiledFunction:

// SourcePos returns the source position of the instruction at ip.
func (o *CompiledFunction) SourcePos(ip int) source.Pos {
	for ip >= 0 {
		if p, ok := o.SourceMap[ip]; ok {
			return p
		}
		ip--
	}
	return source.NoPos
}

, and, change error handling code in VM to:

err = v.err
if err != nil {
	filePos := v.fileSet.Position(v.curFrame.fn.SourcePos(v.ip - 1))
	err = fmt.Errorf("Runtime Error: %s\n\tat %s", err.Error(), filePos)
	for v.framesIndex > 1 {
		v.framesIndex--
		v.curFrame = &v.frames[v.framesIndex-1]

		filePos = v.fileSet.Position(v.curFrame.fn.SourcePos(v.curFrame.ip - 1))
		err = fmt.Errorf("%s\n\tat %s", err.Error(), filePos)
	}
	return err
}

Then, we don't have to track IP offset (.errOffset) and remove it.

@d5
Copy link
Owner

d5 commented Mar 21, 2019

Also, if we keep the proposed panic-recover approach, we can remove stack overflow checks too:

defer func() {
	if r := recover(); r != nil {
		if v.sp >= StackSize || v.framesIndex >= MaxFrames {
			v.err = ErrStackOverflow
			return
		}

		if v.ip < len(v.curInsts)-1 {
			if err, ok := r.(error); ok {
				v.err = err
			} else {
				v.err = fmt.Errorf("panic: %v", r)
			}
		}
	}
}()

runtime/vm.go Outdated
v.binaryOp(token.Shl)
if e == objects.ErrInvalidOperator {
v.err = fmt.Errorf("invalid operation: %s + %s",
left.TypeName(), right.TypeName())
Copy link
Owner

Choose a reason for hiding this comment

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

Should be

fmt.Errorf("invalid operation: %s %s %s",
 	left.TypeName(), tok.String(), right.TypeName())

@d5 d5 merged commit 09f3d52 into d5:master Mar 22, 2019
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.

None yet

2 participants