Pull Requests

[cchamplin]

Hey, just wondering if you guys are looking for/accepting pull requests into the project?

I've done some work on a fork (static casting between v8.Value and native go types and back, wrapped go objects with prototype handlers for functions and property handling). The changes add quite a bit of boilerplate to the code, but should provide more performance around marshaling data back and forth.

Code is available here: https://github.com/cchamplin/go-v8/commits/stage

Not sure if you're interested in that kind of thing but let me know.

[forwidur]

Thanks a lot for the contribution!

Let me go through your fork and see if there are breaking changes for us.

[forwidur]

Our functional tests pass.

But the unittests seem to be failing for me:

--- FAIL: TestInterfaceWrap (0.00s)
panic: runtime error: cgo argument has Go pointer to Go pointer [recovered]
    panic: runtime error: cgo argument has Go pointer to Go pointer

goroutine 834 [running]:
panic(0xadb460, 0xc8208b5b10)
    /usr/local/go/src/runtime/panic.go:464 +0x3e6
testing.tRunner.func1(0xc8206953b0)
    /usr/local/go/src/testing/testing.go:467 +0x192
panic(0xadb460, 0xc8208b5b10)
    /usr/local/go/src/runtime/panic.go:426 +0x4e9
github.com/fluxio/go-v8.(*Prototype).AddMethod(0xc8205f3780, 0xb17aa0, 0xb, 0xc8207834c0)
    /home/mag/work/flux/genie/backend/src/github.com/fluxio/go-v8/v8.go:483 +0x16a
github.com/fluxio/go-v8.TestInterfaceWrap(0xc8206953b0)
    /home/mag/work/flux/genie/backend/src/github.com/fluxio/go-v8/v8_test.go:1023 +0x339
testing.tRunner(0xc8206953b0, 0x106a0f8)
    /usr/local/go/src/testing/testing.go:473 +0x98
created by testing.RunTests
    /usr/local/go/src/testing/testing.go:582 +0x892
exit status 2

Could you take a look?

Thanks again for contributing!

[augustoroman]

Indeed, the problem is
cchamplin@7b78893#commitcomment-17442893

On Wed, May 11, 2016 at 3:27 PM Max Grigorev [email protected]
wrote:

Our functional tests pass.

But the unittests seem to be failing for me:

--- FAIL: TestInterfaceWrap (0.00s)
panic: runtime error: cgo argument has Go pointer to Go pointer [recovered]
panic: runtime error: cgo argument has Go pointer to Go pointer

goroutine 834 [running]:
panic(0xadb460, 0xc8208b5b10)
/usr/local/go/src/runtime/panic.go:464 +0x3e6
testing.tRunner.func1(0xc8206953b0)
/usr/local/go/src/testing/testing.go:467 +0x192
panic(0xadb460, 0xc8208b5b10)
/usr/local/go/src/runtime/panic.go:426 +0x4e9github.com/fluxio/go-v8.(*Prototype).AddMethod(0xc8205f3780, 0xb17aa0, 0xb, 0xc8207834c0)
/home/mag/work/flux/genie/backend/src/github.com/fluxio/go-v8/v8.go:483 +0x16agithub.com/fluxio/go-v8.TestInterfaceWrap(0xc8206953b0)
/home/mag/work/flux/genie/backend/src/github.com/fluxio/go-v8/v8_test.go:1023 +0x339
testing.tRunner(0xc8206953b0, 0x106a0f8)
/usr/local/go/src/testing/testing.go:473 +0x98
created by testing.RunTests
/usr/local/go/src/testing/testing.go:582 +0x892
exit status 2

Could you take a look?

Thanks again for contributing!

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1 (comment)

[cchamplin]

Ah, ya been testing on 1.5. I was hoping to avoid doing a map lookup for the function pointer and wrapper data after we call back into Go (for performance reasons). We're already losing ~160ns every time we traverse between Go and C :-/.

I'll try and get a rework committed in the next day or so. Thanks for the review!

[forwidur]

We're already losing ~160ns every time we traverse between Go and C :-/.

Looks too high, how did you measure that? Should not be more than a 100 or so instructions.

Thanks for the review!

Thanks for doing the work! :-D

[cchamplin]

That figure comes from Russ Cox (https://groups.google.com/d/msg/golang-nuts/NNaluSgkLSU/0bq1kXZueCwJ) and Mikael Gustavsson. It's mostly a result of go establishing a global local every time it crosses the go->c boundary