uint8 array support #143

teuget · 2021-06-08T07:51:35Z

This PR adds code to v8go to convert []uint8 array from javascript to golang and back,
and also an Isolate method to allow throwing javascript exceptions from golang callbacks.
Test case resides in file array_test.go.

codecov · 2021-06-09T04:21:21Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.79 🎉

Comparison is base (1f00b50) 95.23% compared to head (92acee2) 97.03%.

❗ Current head 92acee2 differs from pull request most recent head b75eec2. Consider uploading reports for the commit b75eec2 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   95.23%   97.03%   +1.79%     
==========================================
  Files          17       13       -4     
  Lines         588      472     -116     
==========================================
- Hits          560      458     -102     
+ Misses         19        9      -10     
+ Partials        9        5       -4

Impacted Files	Coverage Δ
arraybuffer.go	`100.00% <100.00%> (ø)`
isolate.go	`100.00% <100.00%> (ø)`
object.go	`100.00% <100.00%> (ø)`
value.go	`97.63% <100.00%> (+2.80%)`	⬆️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tmc

I'd suggest separating the exception support from the uint8 support. Looks like a promising contribution!

tmc · 2021-06-09T18:51:59Z

array_test.go

+	"testing"
+)
+
+type NativeObject interface {


Did you intend this to be part of the public API for the package? if so it shouldn't be in a "*_test.go" file.

This is not a V8 object, so it shouldn't be part of the public API

I not think you need this to be an interface

This code is testing-only, should never have been exported. Will fix.

tmc · 2021-06-09T18:52:30Z

isolate.go

@@ -115,3 +117,10 @@ func (i *Isolate) getCallback(ref int) FunctionCallback {
 	defer i.cbMutex.RUnlock()
 	return i.cbs[ref]
 }
+
+func (i *Isolate) ThrowException(msg string) *Value { // TwinTag added


New exported functions should have a comment.
No need to include TwinTag added here.

Will add missing comment.
The 'TwintTag' thingy is an oversight. I must have missed this one when I removed these tags :-(

rogchap

Why is there changes to the linux lib?

rogchap · 2021-06-10T13:09:20Z

value.go

@@ -82,6 +82,11 @@ func NewValue(iso *Isolate, val interface{}) (*Value, error) {
 		rtnVal = &Value{
 			ptr: C.NewValueNumber(iso.ptr, C.double(v)),
 		}
+	case []uint8: // TwinTag added
+		rtnVal = &Value{
+			//NOTE: C.CBytes() allocates memory, must be freed


How will CBytes be freed

The newly added function C.NewValueUint8Array() creates a BackingStore that takes ownership over the array allocated here by C.CBytes(). This allocation gets freed when V8 decides it can get rid of the BackingStore. Verified manually by adding a print statement to the deallocator.

Would/Could this cause a panic on the Go side? For example V8 removes the backing store, and then the caller tries to read from the Go slice.

The bytes allocated by C.CBytes() are passed directly into the V8 BackingStore that then takes ownership over the allocation. They are not exposed or handed out anywhere else, so it is not possible for any Go code to do an invalid access.

This comment in the code is not very useful at the moment.

What is important from the perspective of this function is that NewValueUint8Array takes ownership of that memory allocation.

Suggested change

//NOTE: C.CBytes() allocates memory, must be freed

// C.NewValueUint8Array takes ownership of the C.CBytes() allocation, making sure it gets freed.

Details of how C.NewValueUint8Array does that matter more in the context of its implementation. I think it is clear enough from looking at that function that it is passing a deletion callback to the backing store to free the memory. If you disagree, please comment on that code.

rogchap · 2021-06-10T13:18:54Z

There is some great code here; but I'm unsure about this API as it's not following the V8 API (in hindsight I think this is also true for BigInt and maybe some of the other simple types)

I would like to see the API reflect the inheritance structure that exits in the V8 API eg: https://v8.github.io/api/head/classv8_1_1Uint8Array.html

Similar to how we deal with Object and Function, Array and ArrayBuffers should follow suit.

Once that API is in place we can add convenance methods between the Go world and the V8 world.

Although there is an overhead, I would avoid shared memory for the slices as it can be hard for callers to free allocated memory; we want the public API to be as Go friendly as possible.

huttarichard · 2021-06-30T10:30:29Z

This is just wonderful PR! Cant wait to have this merged. 👍

huttarichard · 2021-07-06T11:02:33Z

@teuget btw I did messed around with changes. Seems to me that v8go.NewObject doesnt work elsewhere except in the context of NewFunctionTemplate.

try this:

ctx, err := v8go.NewContext()
require.NoError(t, err)
v8go.NewObject(ctx)

fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x1 addr=0xffffffffffffffff pc=0x41a93c9]

runtime stack:
runtime.throw(0x4d2ae0c, 0x2a)
	/usr/local/go/src/runtime/panic.go:1117 +0x72
runtime.sigpanic()
	/usr/local/go/src/runtime/signal_unix.go:718 +0x2ef

goroutine 19 [syscall]:
runtime.cgocall(0x4157330, 0xc00003e6b0, 0x4128f05)
	/usr/local/go/src/runtime/cgocall.go:154 +0x5b fp=0xc00003e680 sp=0xc00003e648 pc=0x4007d1b
rogchap.com/v8go._Cfunc_NewObject(0x357300000000, 0x0)
	_cgo_gotypes.go:431 +0x45 fp=0xc00003e6b0 sp=0xc00003e680 pc=0x4125105
rogchap.com/v8go.NewObject.func1(0xc00012c048, 0xc0001188b0)
	/v8go/object.go:25 +0x59 fp=0xc00003e6e8 sp=0xc00003e6b0 pc=0x41318f9
rogchap.com/v8go.NewObject(0xc00012c048, 0x1)
	/v8go/object.go:25 +0x39 fp=0xc00003e718 sp=0xc00003e6e8 pc=0x412b3d9
rogchap.com/v8go_test.TestNewObject(0xc000102900)
	/v8go/object_test.go:214 +0x7f fp=0xc00003e780 sp=0xc00003e718 pc=0x414d1df
testing.tRunner(0xc000102900, 0x4d2ddb0)
	/usr/local/go/src/testing/testing.go:1193 +0xef fp=0xc00003e7d0 sp=0xc00003e780 pc=0x40d680f
runtime.goexit()
	/usr/local/go/src/runtime/asm_amd64.s:1371 +0x1 fp=0xc00003e7d8 sp=0xc00003e7d0 pc=0x4071781
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1238 +0x2b3

goroutine 1 [chan receive]:
testing.tRunner.func1(0xc000102780)
	/usr/local/go/src/testing/testing.go:1159 +0x2bc
testing.tRunner(0xc000102780, 0xc000139da8)
	/usr/local/go/src/testing/testing.go:1197 +0x125
testing.runTests(0xc00012c030, 0x4f4b7c0, 0x37, 0x37, 0xc0312d4103ceea20, 0x1bf094dc93, 0x4f5af20, 0x4d22a1a)
	/usr/local/go/src/testing/testing.go:1509 +0x2fe
testing.(*M).Run(0xc000172000, 0x0)
	/usr/local/go/src/testing/testing.go:1417 +0x1eb
main.main()
	_testmain.go:249 +0x1c5

goroutine 20 [chan receive]:
testing.runTests.func1.1(0xc000102780)
	/usr/local/go/src/testing/testing.go:1516 +0x3b
created by testing.runTests.func1
	/usr/local/go/src/testing/testing.go:1516 +0xac
FAIL	rogchap.com/v8go	0.445s
FAIL

func TestNewObject(t *testing.T) {
	t.Parallel()
	iso, _ := v8go.NewIsolate()
	ctx, _ := v8go.NewContext(iso)

	obj := v8go.NewObject(ctx)
	err := obj.Set("test", "ok")
	if err != nil {
		t.Errorf("Got error from setting object property: %v", err)
	}
}

huttarichard · 2021-07-06T12:06:39Z

but seems that c->Enter(); and c->Exit(); does the job for NewObject as well.

teuget · 2021-07-06T12:15:47Z

Richard, Great to hear that Enter/Exit fixes the issue. That was also the first thing I was going to try when I got the chance. Thanks, Tom

…

On Tue, 6 Jul 2021 at 14:06, Richard Hutta ***@***.***> wrote: but seems that c->Enter(); and c->Exit(); does the job for NewObject as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#143 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACFKA4726JWGAU5P74J2NCTTWLWVXANCNFSM46JJSGVA> .

dylanahsmith · 2021-09-16T14:38:20Z

array_test.go

@@ -0,0 +1,119 @@
+package v8go


Tests should be in the test package.

Also, this is missing a copyright header that all the other go files have.

Suggested change

package v8go

// Copyright 2021 the v8go contributors. All rights reserved.

// Use of this source code is governed by a BSD-style license that can be

// found in the LICENSE file.

package v8go_test

This will also require importing v8go and using its namespace when accessing its contents.

Lastly, an array_test.go test file would conventionally test code in a corresponding array.go file, but that file doesn't exist.

dylanahsmith · 2021-09-16T14:45:01Z

array_test.go

+	"errors"
+	"fmt"
+	"log"
+	"testing"


Suggested change

"testing"

"testing"

v8 "rogchap.com/v8go"

dylanahsmith · 2021-09-16T14:45:20Z

array_test.go

+	"testing"
+)
+
+func reverseUint8ArrayFunctionCallback(info *FunctionCallbackInfo) *Value {


Suggested change

func reverseUint8ArrayFunctionCallback(info *FunctionCallbackInfo) *Value {

func reverseUint8ArrayFunctionCallback(info *v8.FunctionCallbackInfo) *Value {

dylanahsmith · 2021-09-16T14:50:22Z

array_test.go

+	if val, err := ctx.RunScript("native.reverseUint8Array(new Uint8Array([0,1,2,3,4,5,6,7,8,9]))", ""); err != nil {
+		t.Error(err)
+	} else {


Use fatal errors to avoid needing to deeply nest the non-error paths. There is a test helper method for this

Suggested change

if val, err := ctx.RunScript("native.reverseUint8Array(new Uint8Array([0,1,2,3,4,5,6,7,8,9]))", ""); err != nil {

t.Error(err)

} else {

val, err := ctx.RunScript("native.reverseUint8Array(new Uint8Array([0,1,2,3,4,5,6,7,8,9]))", "")

failIf(t, err)

dylanahsmith · 2021-09-16T14:52:35Z

array_test.go

+		if !val.IsUint8Array() {
+			t.Errorf("Expected uint8 array return value")
+		}
+		fmt.Printf("Reversed array: %v\n", val.Uint8Array())


Avoid polluting the test output with print statements.

Suggested change

fmt.Printf("Reversed array: %v\n", val.Uint8Array())

dylanahsmith · 2021-09-16T16:02:52Z

v8go.cc

+  // The Context::Enter/Exit is only needed when calling this code from low-level unit tests,
+  // otherwise ArrayBuffer::New() trips over missing context.
+  // They are not needed when this code gets called through an executing script.
+  c->Enter();


I think that this is normally done using Context::Scope context_scope(c) so there is no need for the separate Enter and Exit call.

dylanahsmith · 2021-09-17T18:10:15Z

object_test.go

+	}
+	read := args[0].Int32()
+	written := args[1].Int32()
+	obj := v8go.NewObject(info.Context()) // create object


Just like in array_test.go and arraybuffer_test.go, these tests are very indirect making them much less readable by not being focused on what is actually being tested.

dylanahsmith · 2021-09-17T18:19:57Z

arraybuffer.go

+}
+
+func (ab *ArrayBuffer) PutBytes(bytes []uint8) {
+	cbytes := C.CBytes(bytes) //FIXME is there really no way to avoid this malloc+memcpy?


I think we should be able to pass in unsafe.Pointer(&bytes[0]) and len(bytes) to the C function to avoid the extra allocation and copy.

dylanahsmith · 2021-09-17T18:29:35Z

arraybuffer.go

+	return int64(C.ArrayBufferByteLength(ab.ptr))
+}
+
+func (ab *ArrayBuffer) GetBytes() []uint8 {


Prefixing methods with Get isn't idiomatic for Go.

@dylanahsmith can you expand on this? I want to make sure I do this correctly in the future. Do you mean that it should just be Bytes()?

Do you mean that it should just be Bytes()?

Yes

dylanahsmith · 2021-09-17T18:56:01Z

arraybuffer.go

+	len := C.ArrayBufferByteLength(ab.ptr)
+	cbytes := unsafe.Pointer(C.GetArrayBufferBytes(ab.ptr)) // points into BackingStore
+	return C.GoBytes(cbytes, C.int(len))


We can't safely just expose the C++ data. However, we could expose more primitive operations. For instance, if we exposed a method to append the data to a Go slice, then we could implement a method that does this in pure Go using that on an empty slice.

Another possible primitive would be something like v8::ArrayBufferView::CopyContents which could be used to copy as much as it could into a slice without growing it for the fixed size buffer case. It might also be useful to be able to get something like a V8::ArrayBufferView without having to allocate the V8 object for it, in order to specify the part of the ArrayBuffer to copy/append into or out of a Go slice.

I suppose more primitive operations could always be added later though.

boindil · 2022-03-03T11:08:58Z

@sharmaashish13 @teuget are you still on this? I think this is a great addition and would make this library so much better. I actually need workarounds for this as of now and this being finished would be a charm! ❤️

ti2ger92 · 2022-05-29T03:33:31Z

@sharmaashish13 @teuget I agree with @boindil . I'd also dig String arrays.

RGood · 2022-05-30T01:23:38Z

+1 in wanting to see this get merged

prodikos · 2022-07-13T09:47:04Z

+1 here 😅

…tep: test that we can pass a go []uint8 to js.

…reverses a uint8 array via a function implemented in go. Still to be scrutinized: memory management, error reporting.

…nctions.

…rrayBuffer).

…n. This commit allows Go code to directly access the raw ArrayBuffer bytes. A next step could be to add typed View objects to mirror those found in JS. Feedback welcome.

tmc reviewed Jun 9, 2021

View reviewed changes

rogchap reviewed Jun 10, 2021

View reviewed changes

dylanahsmith requested changes Sep 17, 2021

View reviewed changes

wisepythagoras mentioned this pull request Oct 31, 2021

Add the ThrowException fn to the isolate #221

Merged

sharmaashish13 force-pushed the master branch from 92acee2 to 40a3423 Compare December 28, 2021 13:16

sharmaashish13 force-pushed the master branch 2 times, most recently from e01fad2 to 4c49aa9 Compare January 10, 2022 09:20

This was referenced Dec 16, 2022

Support for PumpMessageLoop, needed for webassembly #355

Open

Support []byte in Value #356

Open

teuget added 10 commits January 2, 2023 20:33

NewValueUint8Array() working (at least, not crashing anymore). Next s…

e2774a2

…tep: test that we can pass a go []uint8 to js.

First working end-to-end test of []uint8 support in v8go. Javascript …

07bd22d

…reverses a uint8 array via a function implemented in go. Still to be scrutinized: memory management, error reporting.

Code cleanup, memory management.

58f8993

Implemented + tested throwing exceptions from natively implemented fu…

75ee784

…nctions.

More checks in unit test + code cleanup

5aabb67

Final changes uint8 array support (switch to using BackingStore for A…

219118d

…rrayBuffer).

Removing obsolete editing comments + accidental go mod dependency.

ab2e0c3

Replace formatting directive %w by %v, build seems to fail over it.

9e35bfe

Adding ArrayBuffer support along the same lines as Object and Functio…

6f7fabf

…n. This commit allows Go code to directly access the raw ArrayBuffer bytes. A next step could be to add typed View objects to mirror those found in JS. Feedback welcome.

Added support for creating objects in golang function callbacks.

3e88861

teuget and others added 3 commits January 2, 2023 20:34

Remove unnecessary cruft from the tests injecting function callbacks.

e176fa9

build fix after latest rebase

49caa52

removed print statement

fab79d7

sharmaashish13 force-pushed the master branch from d38fc1d to fab79d7 Compare January 2, 2023 15:10

Upgrade V8 binaries for 11.1.277.13 version

b75eec2

	//NOTE: C.CBytes() allocates memory, must be freed
	// C.NewValueUint8Array takes ownership of the C.CBytes() allocation, making sure it gets freed.

-package v8go
+// Copyright 2021 the v8go contributors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+package v8go_test

	func reverseUint8ArrayFunctionCallback(info FunctionCallbackInfo) Value {
	func reverseUint8ArrayFunctionCallback(info v8.FunctionCallbackInfo) Value {

uint8 array support #143

Are you sure you want to change the base?

uint8 array support #143

Conversation

teuget commented Jun 8, 2021 • edited Loading

codecov bot commented Jun 9, 2021 • edited Loading

Codecov Report

tmc 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

rogchap 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

rogchap commented Jun 10, 2021

huttarichard commented Jun 30, 2021

huttarichard commented Jul 6, 2021 • edited Loading

huttarichard commented Jul 6, 2021

teuget commented Jul 6, 2021 via email

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boindil commented Mar 3, 2022

ti2ger92 commented May 29, 2022 • edited Loading

RGood commented May 30, 2022

prodikos commented Jul 13, 2022

teuget commented Jun 8, 2021 •

edited

Loading

codecov bot commented Jun 9, 2021 •

edited

Loading

huttarichard commented Jul 6, 2021 •

edited

Loading

ti2ger92 commented May 29, 2022 •

edited

Loading