Introduce the Array Buffer C API #2161

galpeter · 2018-01-05T17:48:22Z

Add C API to work with Array Buffers.
The following methods are added:

jerry_value_is_arraybuffer
jerry_create_arraybuffer
jerry_arraybuffer_write
jerry_arraybuffer_read
jerry_get_arraybuffer_byte_length

JerryScript-DCO-1.0-Signed-off-by: Peter Gal [email protected]

galpeter · 2018-01-05T17:54:36Z

As per #1832 I've created an API proposal for the Array Buffers.

Some notes:

The jerry_arraybuffer_map call is a bit dangerous but useful in case we want to reduce memory usage with array buffers. I'm not sure we want this.
I was wondering on a way to have the array buffer use an external (non-jerry heap memory) for which we could have the map operation. I'll try to dig around this a bit.
The names of the functions was changed a bit, but maybe there are better names.

robertsipka · 2018-01-08T11:22:18Z

docs/02.API-REFERENCE.md

+there is no out of bounds reads or writes.
+
+After using the pointer the [jerry_arraybuffer_unmap][#jerry_arraybuffer_unmap)
+function must be called.


Wrong bracket

robertsipka · 2018-01-08T11:22:53Z

docs/02.API-REFERENCE.md

+**Summary**
+
+Release the Array Buffer after a [jerry_arraybuffer_map][#jerry_arraybuffer_map)
+function call.


robertsipka · 2018-01-08T11:32:32Z

docs/02.API-REFERENCE.md

+```c
+{
+  jerry_value_t buffer = jerry_create_arraybuffer (22);
+  uint8_t * const data = jerry_arraybuffer_map (buffer);


Put a space only before the asterisk character

robertsipka · 2018-01-08T11:33:02Z

jerry-core/api/jerry.c

+
+  ecma_object_t *buffer_p = ecma_get_object_from_value (buffer);
+  lit_utf8_byte_t *mem_buffer_p = ecma_arraybuffer_get_buffer (buffer_p);
+  return (uint8_t * const) mem_buffer_p;


robertsipka · 2018-01-08T11:33:04Z

docs/02.API-REFERENCE.md

+  // acquire the Array Buffer from somewhere
+
+  uint8_t * const data = jerry_arraybuffer_map (array_buffer);
+


robertsipka · 2018-01-08T11:33:13Z

jerry-core/include/jerryscript-core.h

+                                       uint8_t *buf_p,
+                                       jerry_length_t buf_size);
+jerry_length_t jerry_arraybuffer_get_length (const jerry_value_t value);
+uint8_t * jerry_arraybuffer_map (const jerry_value_t value);


robertsipka · 2018-01-08T11:33:25Z

tests/unit-core/test-arraybuffer.c

+    TEST_ASSERT (jerry_value_is_arraybuffer (buffer));
+    TEST_ASSERT (jerry_arraybuffer_get_length (buffer) == 20);
+
+    uint8_t * const data = jerry_arraybuffer_map (buffer);


robertsipka

LGTM (informally) after these minor fixes.

robertsipka · 2018-01-08T11:50:43Z

docs/02.API-REFERENCE.md

+
+  jerry_value_t buffer;
+  // ... create the Array Buffer or acuiqre it from somewhere.
+


robertsipka · 2018-01-08T11:51:01Z

docs/02.API-REFERENCE.md

+
+**Summary**
+
+The function allows acces to the contents of the Array Buffer directly.


galpeter · 2018-01-08T14:48:19Z

I've updated the PR with the requested changes. Also I've removed the _map and _unmap functions as they are high-risk functions and I'll will create another PR which introduces ArrayBuffers with external (non Jerry heap) memory usage.

zherczeg · 2018-01-09T07:41:41Z

docs/02.API-REFERENCE.md

+
+```c
+jerry_length_t
+jerry_arraybuffer_get_length (const jerry_value_t value);


There is a jerry_get_array_length function already. I am thinking whether this could be combined with it. Maybe just introducing a jerry_get_length() function which works with any special object (list in the comment what types it supports). But that could increase the binary size. I am unsure.

The jerry_arraybuffer_get_length returns the byteLength property (which is specified ad construction time) and the arraybuffer does not have a length property. I'm not really sure we should remove this function, but we could change the name to have byte_length suffix?

Maybe a name like: jerry_get_arraybuffer_byte_length ? so we'll have a common naming scheme for "getters".

Hm, actually sounds good.

zherczeg · 2018-01-09T07:43:27Z

docs/02.API-REFERENCE.md

+
+**Summary**
+
+Read the contents of the Array Buffer into a buffer.


Copy the portion of the Array Buffer into a user provided buffer.

zherczeg · 2018-01-09T07:45:34Z

docs/02.API-REFERENCE.md

+
+**Summary**
+
+Write the contents of a buffer into the Array Buffer.


zherczeg · 2018-01-09T07:45:58Z

docs/02.API-REFERENCE.md

+jerry_arraybuffer_write (const jerry_value_t value,
+                        jerry_length_t offset,
+                        const uint8_t *buf_p,
+                        jerry_length_t buf_size);


wrong alignment

zherczeg · 2018-01-09T07:50:16Z

jerry-core/api/jerry.c

+
+  ecma_object_t *buffer_p = ecma_get_object_from_value (buffer);
+  jerry_length_t length = ecma_arraybuffer_get_length (buffer_p);
+  jerry_length_t copy_count = JERRY_MIN (length - offset, buf_size);


What happens it offset > length? In some range, it will be a big number, and the min mitigates the issue, but if offset is 0xffffffff that could be exploited.

I would add an early return if offset >= length.

zherczeg · 2018-01-09T07:50:53Z

jerry-core/api/jerry.c

+
+  ecma_object_t *buffer_p = ecma_get_object_from_value (buffer);
+  jerry_length_t length = ecma_arraybuffer_get_length (buffer_p);
+  jerry_length_t copy_count = JERRY_MIN (length - offset, buf_size);


offset issue again

galpeter · 2018-01-09T15:17:24Z

@zherczeg I've updated the PR with the requested changes, and also added two new api tests.

zherczeg

Few changes. Patch is close to ready.

zherczeg · 2018-01-10T08:32:06Z

docs/02.API-REFERENCE.md

+jerry_create_arraybuffer (jerry_length_t size);
+```
+
+ - `size` - size of the ArrayBuffer to create


byte size. I think we need to emphasize this.

zherczeg · 2018-01-10T08:35:31Z

tests/unit-core/test-arraybuffer.c

+  {
+    return jerry_create_boolean (true);
+  }
+  else


No need else after return.

Add C API to work with Array Buffers. The following methods are added: - jerry_value_is_arraybuffer - jerry_create_arraybuffer - jerry_arraybuffer_write - jerry_arraybuffer_read - jerry_get_arraybuffer_byte_length JerryScript-DCO-1.0-Signed-off-by: Peter Gal [email protected]

galpeter · 2018-01-11T11:31:49Z

@zherczeg updated PR based on requests.

zherczeg

LGTM

martijnthe · 2018-01-11T13:48:09Z

Hey @galpeter, thanks for adding this. Looks great.
The one thing I'm missing is a non-copy read API (basically getting a pointer to the internal buffer).
The need for this was raised by multiple people in the discussion here:
#1832
Fine to add it later, I just wanted to bring it up again and make sure there's nothing in the way to do so later.

zherczeg · 2018-01-11T13:54:13Z

Please check #2162 . The idea is that we allow arraybuffer to be created for external byte arrays. This allows communication between jerryscript instances, direct access for screen memory, etc. However there is no plan for accessing data of internally created arraybuffers.

martijnthe · 2018-01-11T13:56:34Z

@zherczeg got it, makes sense, thanks!

galpeter · 2018-01-11T17:58:36Z

@martijnthe initially I had that non-copy api which returned the internal buffer pointer, but that was to risky so I've changed the logic to external buffers which can be seen in #2162.

LaszloLango

LGTM

galpeter added api Related to the public API enhancement An improvement ES2015 Related to ES2015 features labels Jan 5, 2018

robertsipka reviewed Jan 8, 2018

View reviewed changes

galpeter force-pushed the c-api-arraybuffer branch from 985816f to 3cd07f6 Compare January 8, 2018 14:45

galpeter added the feature request Requested feature label Jan 8, 2018

galpeter mentioned this pull request Jan 8, 2018

Add ArrayBuffer with user specified buffer #2162

Merged

zherczeg requested changes Jan 9, 2018

View reviewed changes

galpeter force-pushed the c-api-arraybuffer branch 2 times, most recently from d4e26eb to ffea5b8 Compare January 9, 2018 15:16

zherczeg requested changes Jan 10, 2018

View reviewed changes

galpeter force-pushed the c-api-arraybuffer branch from ffea5b8 to 4d76a8c Compare January 11, 2018 11:27

galpeter force-pushed the c-api-arraybuffer branch from 4d76a8c to c90f081 Compare January 11, 2018 11:28

zherczeg approved these changes Jan 11, 2018

View reviewed changes

LaszloLango approved these changes Jan 11, 2018

View reviewed changes

LaszloLango merged commit ded0d5a into jerryscript-project:master Jan 11, 2018

zherczeg mentioned this pull request Mar 27, 2018

Nominating Robert Sipka (robertsipka) to become a reviewer #2254

Closed

		// acquire the Array Buffer from somewhere

		uint8_t * const data = jerry_arraybuffer_map (array_buffer);


		jerry_value_t buffer;
		// ... create the Array Buffer or acuiqre it from somewhere.


		Summary

		The function allows acces to the contents of the Array Buffer directly.


		Summary

		Read the contents of the Array Buffer into a buffer.


		Summary

		Write the contents of a buffer into the Array Buffer.

Introduce the Array Buffer C API #2161

Introduce the Array Buffer C API #2161

Uh oh!

Conversation

galpeter commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

galpeter commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsipka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galpeter commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galpeter commented Jan 9, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galpeter commented Jan 11, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

martijnthe commented Jan 11, 2018

Uh oh!

zherczeg commented Jan 11, 2018

Uh oh!

martijnthe commented Jan 11, 2018

Uh oh!

galpeter commented Jan 11, 2018

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

galpeter commented Jan 5, 2018 •

edited

Loading