From 8c3f3303f20ab5f1031731af6df5c2ef1499cd36 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?=
Date: Tue, 20 Feb 2018 11:30:09 +0100
Subject: Add a helper function for attaching tmp objects to NIF environments
---
erts/emulator/beam/erl_nif.c | 96 ++++++++++++++++++++++++++------------------
1 file changed, 58 insertions(+), 38 deletions(-)
diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c
index 7db4af8919..6ccf37e793 100644
--- a/erts/emulator/beam/erl_nif.c
+++ b/erts/emulator/beam/erl_nif.c
@@ -237,9 +237,11 @@ static void cache_env(ErlNifEnv* env);
static void full_flush_env(ErlNifEnv *env);
static void flush_env(ErlNifEnv* env);
-/* Temporary object header, auto-deallocated when NIF returns
- * or when independent environment is cleared.
- */
+/* Temporary object header, auto-deallocated when NIF returns or when
+ * independent environment is cleared.
+ *
+ * The payload can be accessed with &tmp_obj_ptr[1] but keep in mind that its
+ * first element must not require greater alignment than `next`. */
struct enif_tmp_obj_t {
struct enif_tmp_obj_t* next;
void (*dtor)(struct enif_tmp_obj_t*);
@@ -256,6 +258,46 @@ static ERTS_INLINE void free_tmp_objs(ErlNifEnv* env)
}
}
+/* Whether the given environment is bound to a process and will be cleaned up
+ * when the NIF returns. It's safe to use temp_alloc for objects in
+ * env->tmp_obj_list when this is true. */
+static ERTS_INLINE int is_proc_bound(ErlNifEnv *env)
+{
+ return env->mod_nif != NULL;
+}
+
+/* Allocates and attaches an object to the given environment, running its
+ * destructor when the environment is cleared. To avoid temporary variables the
+ * address of the allocated object is returned instead of the enif_tmp_obj_t.
+ *
+ * The destructor *must* call `erts_free(tmp_obj->allocator, tmp_obj)` to free
+ * the object. If the destructor needs to refer to the allocated object its
+ * address will be &tmp_obj[1]. */
+static ERTS_INLINE void *alloc_tmp_obj(ErlNifEnv *env, size_t size,
+ void (*dtor)(struct enif_tmp_obj_t*)) {
+ struct enif_tmp_obj_t *tmp_obj;
+ ErtsAlcType_t allocator;
+
+ allocator = is_proc_bound(env) ? ERTS_ALC_T_TMP : ERTS_ALC_T_NIF;
+
+ tmp_obj = erts_alloc(allocator, sizeof(struct enif_tmp_obj_t) + MAX(1, size));
+
+ tmp_obj->next = env->tmp_obj_list;
+ tmp_obj->allocator = allocator;
+ tmp_obj->dtor = dtor;
+
+ env->tmp_obj_list = tmp_obj;
+
+ return (void*)&tmp_obj[1];
+}
+
+/* Generic destructor for objects allocated through alloc_tmp_obj that don't
+ * care about their payload. */
+static void tmp_alloc_dtor(struct enif_tmp_obj_t *tmp_obj)
+{
+ erts_free(tmp_obj->allocator, tmp_obj);
+}
+
void erts_post_nif(ErlNifEnv* env)
{
erts_unblock_fpe(env->fpe_was_unmasked);
@@ -1019,11 +1061,6 @@ int enif_is_number(ErlNifEnv* env, ERL_NIF_TERM term)
return is_number(term);
}
-static ERTS_INLINE int is_proc_bound(ErlNifEnv* env)
-{
- return env->mod_nif != NULL;
-}
-
static void aligned_binary_dtor(struct enif_tmp_obj_t* obj)
{
erts_free_aligned_binary_bytes_extra((byte*)obj, obj->allocator);
@@ -1065,15 +1102,8 @@ int enif_inspect_binary(ErlNifEnv* env, Eterm bin_term, ErlNifBinary* bin)
return 1;
}
-static void tmp_alloc_dtor(struct enif_tmp_obj_t* obj)
-{
- erts_free(obj->allocator, obj);
-}
-
int enif_inspect_iolist_as_binary(ErlNifEnv* env, Eterm term, ErlNifBinary* bin)
{
- struct enif_tmp_obj_t* tobj;
- ErtsAlcType_t allocator;
ErlDrvSizeT sz;
if (is_binary(term)) {
return enif_inspect_binary(env,term,bin);
@@ -1089,14 +1119,7 @@ int enif_inspect_iolist_as_binary(ErlNifEnv* env, Eterm term, ErlNifBinary* bin)
return 0;
}
- allocator = is_proc_bound(env) ? ERTS_ALC_T_TMP : ERTS_ALC_T_NIF;
- tobj = erts_alloc(allocator, sz + sizeof(struct enif_tmp_obj_t));
- tobj->allocator = allocator;
- tobj->next = env->tmp_obj_list;
- tobj->dtor = &tmp_alloc_dtor;
- env->tmp_obj_list = tobj;
-
- bin->data = (unsigned char*) &tobj[1];
+ bin->data = alloc_tmp_obj(env, sz, &tmp_alloc_dtor);
bin->size = sz;
bin->bin_term = THE_NON_VALUE;
bin->ref_bin = NULL;
@@ -4236,34 +4259,31 @@ static unsigned calc_checksum(unsigned char* ptr, unsigned size);
struct readonly_check_t
{
- struct enif_tmp_obj_t hdr;
unsigned char* ptr;
unsigned size;
unsigned checksum;
};
static void add_readonly_check(ErlNifEnv* env, unsigned char* ptr, unsigned sz)
{
- ErtsAlcType_t allocator = is_proc_bound(env) ? ERTS_ALC_T_TMP : ERTS_ALC_T_NIF;
- struct readonly_check_t* obj = erts_alloc(allocator,
- sizeof(struct readonly_check_t));
- obj->hdr.allocator = allocator;
- obj->hdr.next = env->tmp_obj_list;
- env->tmp_obj_list = &obj->hdr;
- obj->hdr.dtor = &readonly_check_dtor;
+ struct readonly_check_t* obj;
+
+ obj = alloc_tmp_obj(env, sizeof(struct readonly_check_t),
+ &readonly_check_dtor);
+
obj->ptr = ptr;
obj->size = sz;
- obj->checksum = calc_checksum(ptr, sz);
+ obj->checksum = calc_checksum(ptr, sz);
}
-static void readonly_check_dtor(struct enif_tmp_obj_t* o)
+static void readonly_check_dtor(struct enif_tmp_obj_t* tmp_obj)
{
- struct readonly_check_t* obj = (struct readonly_check_t*) o;
- unsigned chksum = calc_checksum(obj->ptr, obj->size);
- if (chksum != obj->checksum) {
+ struct readonly_check_t* ro_check = (struct readonly_check_t*)&tmp_obj[1];
+ unsigned chksum = calc_checksum(ro_check->ptr, ro_check->size);
+ if (chksum != ro_check->checksum) {
fprintf(stderr, "\r\nReadonly data written by NIF, checksums differ"
- " %x != %x\r\nABORTING\r\n", chksum, obj->checksum);
+ " %x != %x\r\nABORTING\r\n", chksum, ro_check->checksum);
abort();
}
- erts_free(obj->hdr.allocator, obj);
+ erts_free(tmp_obj->allocator, tmp_obj);
}
static unsigned calc_checksum(unsigned char* ptr, unsigned size)
{
--
cgit v1.2.3
From 7fb9b59ed1fc6cbff581effaea7504fd2dc90f4e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?=
Date: Mon, 19 Feb 2018 17:06:35 +0100
Subject: Free temporary iovecs through the tmp object list
Attaching to a ProcBin is the fastest way to delay the release of an
already allocated Binary, but alloc_tmp_obj is a lot faster when
starting from scratch since it usually uses temp_alloc.
---
erts/emulator/beam/erl_nif.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c
index 6ccf37e793..28b68c9e7d 100644
--- a/erts/emulator/beam/erl_nif.c
+++ b/erts/emulator/beam/erl_nif.c
@@ -3543,19 +3543,14 @@ static int create_iovec_from_slice(ErlNifEnv *env,
alloc_size = binv_offset;
alloc_size += slice->iovec_len * sizeof(Binary*);
- /* If we have an environment we'll attach the allocated data to it. The
- * GC will take care of releasing it later on. */
+ /* When the user passes an environment, we attach the iovec to it so
+ * the user won't have to bother managing it (similar to
+ * enif_inspect_binary). It'll disappear once the environment is
+ * cleaned up. */
if (env != NULL) {
- ErlNifBinary gc_bin;
-
- if (!enif_alloc_binary(alloc_size, &gc_bin)) {
- return 0;
- }
-
- alloc_base = (char*)gc_bin.data;
- enif_make_binary(env, &gc_bin);
+ alloc_base = alloc_tmp_obj(env, alloc_size, &tmp_alloc_dtor);
} else {
- alloc_base = enif_alloc(alloc_size);
+ alloc_base = erts_alloc(ERTS_ALC_T_NIF, alloc_size);
}
iovec = (ErlNifIOVec*)alloc_base;
@@ -3569,7 +3564,7 @@ static int create_iovec_from_slice(ErlNifEnv *env,
if(!fill_iovec_with_slice(env, slice, iovec)) {
if (env == NULL && !(iovec->flags & ERL_NIF_IOVEC_FLAGS_PREALLOC)) {
- enif_free(iovec);
+ erts_free(ERTS_ALC_T_NIF, iovec);
}
return 0;
--
cgit v1.2.3
From 97ac7f37c93b36617fe09b81f64a967b0d341673 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?=
Date: Mon, 19 Feb 2018 18:04:01 +0100
Subject: Tweak asserts and explain why the copy buf doesn't use the tmp obj
list
---
erts/emulator/beam/erl_nif.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c
index 28b68c9e7d..72bff49f32 100644
--- a/erts/emulator/beam/erl_nif.c
+++ b/erts/emulator/beam/erl_nif.c
@@ -3431,6 +3431,7 @@ static void marshal_iovec_binary(Eterm binary, ErlNifBinary *copy_buffer,
* and reference that instead. */
if (result->ref_bin == NULL || bit_offset != 0) {
+ ASSERT(copy_buffer->ref_bin != NULL && copy_buffer->data != NULL);
ASSERT(result->size <= (copy_buffer->size - *copy_offset));
if (bit_offset == 0) {
@@ -3452,8 +3453,8 @@ static void marshal_iovec_binary(Eterm binary, ErlNifBinary *copy_buffer,
static int fill_iovec_with_slice(ErlNifEnv *env,
iovec_slice_t *slice,
ErlNifIOVec *iovec) {
+ ErlNifBinary copy_buffer = {0};
UWord copy_offset, iovec_idx;
- ErlNifBinary copy_buffer;
Eterm sublist_iterator;
/* Set up a common refc binary for all on-heap and unaligned binaries. */
@@ -3461,11 +3462,8 @@ static int fill_iovec_with_slice(ErlNifEnv *env,
if (!enif_alloc_binary(slice->copied_size, ©_buffer)) {
return 0;
}
- } else {
-#ifdef DEBUG
- copy_buffer.data = NULL;
- copy_buffer.size = 0;
-#endif
+
+ ASSERT(copy_buffer.ref_bin != NULL);
}
sublist_iterator = slice->sublist_start;
@@ -3515,9 +3513,11 @@ static int fill_iovec_with_slice(ErlNifEnv *env,
}
} else {
if (slice->copied_size > 0) {
- /* Attach the binary to our environment and let the GC take care of
- * it after returning. */
- enif_make_binary(env, ©_buffer);
+ /* Attach the binary to our environment and let the next minor GC
+ * get rid of it. This is slightly faster than using the tmp object
+ * list since it avoids off-heap allocations. */
+ erts_build_proc_bin(&MSO(env->proc),
+ alloc_heap(env, PROC_BIN_SIZE), copy_buffer.ref_bin);
}
}
--
cgit v1.2.3
From 89de89e4c962aac2ff0c55f420ef6d510533f02a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?=
Date: Mon, 19 Feb 2018 18:04:01 +0100
Subject: Unconditionally transfer ownership to the created term
This fixes two corner-cases:
1) We will no longer return an invalid term when a binary inspected
on environment A is used in enif_make_binary on environment B
2) A double-free in this sequence of events:
* enif_alloc_binary(size, &bin);
* enif_ioq_enq_binary(ioq, &bin, skip);
* enif_make_binary(env, &bin);
* enif_make_binary(env, &bin);
OTP-14931
OTP-14932
---
erts/emulator/beam/erl_nif.c | 42 +++++++++++++++---------------------------
erts/emulator/beam/erl_nif.h | 3 ++-
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c
index 72bff49f32..42a90f706b 100644
--- a/erts/emulator/beam/erl_nif.c
+++ b/erts/emulator/beam/erl_nif.c
@@ -1095,7 +1095,6 @@ int enif_inspect_binary(ErlNifEnv* env, Eterm bin_term, ErlNifBinary* bin)
u.tmp->dtor = &aligned_binary_dtor;
env->tmp_obj_list = u.tmp;
}
- bin->bin_term = bin_term;
bin->size = binary_size(bin_term);
bin->ref_bin = NULL;
ADD_READONLY_CHECK(env, bin->data, bin->size);
@@ -1111,7 +1110,6 @@ int enif_inspect_iolist_as_binary(ErlNifEnv* env, Eterm term, ErlNifBinary* bin)
if (is_nil(term)) {
bin->data = (unsigned char*) &bin->data; /* dummy non-NULL */
bin->size = 0;
- bin->bin_term = THE_NON_VALUE;
bin->ref_bin = NULL;
return 1;
}
@@ -1121,7 +1119,6 @@ int enif_inspect_iolist_as_binary(ErlNifEnv* env, Eterm term, ErlNifBinary* bin)
bin->data = alloc_tmp_obj(env, sz, &tmp_alloc_dtor);
bin->size = sz;
- bin->bin_term = THE_NON_VALUE;
bin->ref_bin = NULL;
erts_iolist_to_buf(term, (char*) bin->data, sz);
ADD_READONLY_CHECK(env, bin->data, bin->size);
@@ -1139,7 +1136,6 @@ int enif_alloc_binary(size_t size, ErlNifBinary* bin)
bin->size = size;
bin->data = (unsigned char*) refbin->orig_bytes;
- bin->bin_term = THE_NON_VALUE;
bin->ref_bin = refbin;
return 1;
}
@@ -1173,12 +1169,10 @@ void enif_release_binary(ErlNifBinary* bin)
{
if (bin->ref_bin != NULL) {
Binary* refbin = bin->ref_bin;
- ASSERT(bin->bin_term == THE_NON_VALUE);
erts_bin_release(refbin);
}
#ifdef DEBUG
bin->data = NULL;
- bin->bin_term = THE_NON_VALUE;
bin->ref_bin = NULL;
#endif
}
@@ -1335,29 +1329,24 @@ int enif_get_string(ErlNifEnv *env, ERL_NIF_TERM list, char* buf, unsigned len,
Eterm enif_make_binary(ErlNifEnv* env, ErlNifBinary* bin)
{
- if (bin->bin_term != THE_NON_VALUE) {
- return bin->bin_term;
- }
- else if (bin->ref_bin != NULL) {
- Binary* bptr = bin->ref_bin;
- Eterm bin_term;
-
+ Eterm bin_term;
+
+ if (bin->ref_bin != NULL) {
+ Binary* binary = bin->ref_bin;
+
bin_term = erts_build_proc_bin(&MSO(env->proc),
alloc_heap(env, PROC_BIN_SIZE),
- bptr);
- if (erts_refc_read(&bptr->intern.refc, 1) == 1) {
- /* Total ownership transfer */
- bin->ref_bin = NULL;
- bin->bin_term = bin_term;
- }
- return bin_term;
- }
- else {
- flush_env(env);
- bin->bin_term = new_binary(env->proc, bin->data, bin->size);
- cache_env(env);
- return bin->bin_term;
+ binary);
+
+ /* Our (possibly shared) ownership has been transferred to the term. */
+ bin->ref_bin = NULL;
+ } else {
+ flush_env(env);
+ bin_term = new_binary(env->proc, bin->data, bin->size);
+ cache_env(env);
}
+
+ return bin_term;
}
Eterm enif_make_sub_binary(ErlNifEnv* env, ERL_NIF_TERM bin_term,
@@ -3403,7 +3392,6 @@ static void marshal_iovec_binary(Eterm binary, ErlNifBinary *copy_buffer,
parent_header = binary_val(parent_binary);
result->size = binary_size(binary);
- result->bin_term = binary;
if (thing_subtag(*parent_header) == REFC_BINARY_SUBTAG) {
ProcBin *pb = (ProcBin*)parent_header;
diff --git a/erts/emulator/beam/erl_nif.h b/erts/emulator/beam/erl_nif.h
index 053f7673c4..eb69a8255c 100644
--- a/erts/emulator/beam/erl_nif.h
+++ b/erts/emulator/beam/erl_nif.h
@@ -137,8 +137,9 @@ typedef struct
unsigned char* data;
/* Internals (avert your eyes) */
- ERL_NIF_TERM bin_term;
void* ref_bin;
+ /* for future additions to be ABI compatible (same struct size) */
+ void* __spare__[1];
}ErlNifBinary;
#if (defined(__WIN32__) || defined(_WIN32) || defined(_WIN32_))
--
cgit v1.2.3
From 24b67adabd6e31ae8f2bb529d4f1cd5d6fdf1b1c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?=
Date: Tue, 20 Feb 2018 19:07:46 +0100
Subject: Expand ErlNifBinary by another word for future additions
When fixing OTP-14931/14932 we had to settle for a solution that
decreased performance in a few corner cases like multiple calls to
enif_make_binary, as the other options on the table required extra
fields which would have broken ABI compatibility.
This is ABI-compatible since the fields are unused and at the end.
---
erts/emulator/beam/erl_nif.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/erts/emulator/beam/erl_nif.h b/erts/emulator/beam/erl_nif.h
index eb69a8255c..a99b4db705 100644
--- a/erts/emulator/beam/erl_nif.h
+++ b/erts/emulator/beam/erl_nif.h
@@ -139,7 +139,7 @@ typedef struct
/* Internals (avert your eyes) */
void* ref_bin;
/* for future additions to be ABI compatible (same struct size) */
- void* __spare__[1];
+ void* __spare__[2];
}ErlNifBinary;
#if (defined(__WIN32__) || defined(_WIN32) || defined(_WIN32_))
--
cgit v1.2.3
From a92c200af89b9b1f35db26ae8e13333af5638e15 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?=
Date: Mon, 19 Feb 2018 18:04:01 +0100
Subject: Make enif_make_binary return heap binaries if possible
This does not avoid allocating a ProcBin since we still need to
release the binary somehow (and using the tmp obj list is slightly
slower since it's an unavoidable off-heap allocation), but it does give
us a hard limit on how long said ProcBin will live, which is a lot
better than before.
OTP-14845
---
erts/emulator/beam/erl_nif.c | 34 +++++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c
index 42a90f706b..d2000aa71e 100644
--- a/erts/emulator/beam/erl_nif.c
+++ b/erts/emulator/beam/erl_nif.c
@@ -488,6 +488,7 @@ static void cache_env(ErlNifEnv* env)
env->hp_end = env->heap_frag->mem + env->heap_frag->alloc_size;
}
}
+
void* enif_priv_data(ErlNifEnv* env)
{
return env->mod_nif->priv_data;
@@ -1334,9 +1335,36 @@ Eterm enif_make_binary(ErlNifEnv* env, ErlNifBinary* bin)
if (bin->ref_bin != NULL) {
Binary* binary = bin->ref_bin;
- bin_term = erts_build_proc_bin(&MSO(env->proc),
- alloc_heap(env, PROC_BIN_SIZE),
- binary);
+ /* If the binary is smaller than the heap binary limit we'll return a
+ * heap binary to reduce the number of small refc binaries in the
+ * system. We can't simply release the refc binary right away however;
+ * the documentation states that the binary should be considered
+ * read-only from this point on, which implies that it should still be
+ * readable.
+ *
+ * We could keep it alive until we return by adding it to the temporary
+ * object list, but that requires an off-heap allocation which is
+ * potentially quite slow, so we create a dummy ProcBin instead and
+ * rely on the next minor GC to get rid of it. */
+ if (bin->size <= ERL_ONHEAP_BIN_LIMIT) {
+ ErlHeapBin* hb;
+
+ hb = (ErlHeapBin*)alloc_heap(env, heap_bin_size(bin->size));
+ hb->thing_word = header_heap_bin(bin->size);
+ hb->size = bin->size;
+
+ sys_memcpy(hb->data, bin->data, bin->size);
+
+ erts_build_proc_bin(&MSO(env->proc),
+ alloc_heap(env, PROC_BIN_SIZE),
+ binary);
+
+ bin_term = make_binary(hb);
+ } else {
+ bin_term = erts_build_proc_bin(&MSO(env->proc),
+ alloc_heap(env, PROC_BIN_SIZE),
+ binary);
+ }
/* Our (possibly shared) ownership has been transferred to the term. */
bin->ref_bin = NULL;
--
cgit v1.2.3
From 165b8455d4bf2e3797008a9ad8496b203d017474 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?=
Date: Mon, 19 Feb 2018 19:40:10 +0100
Subject: Suggest using enif_make_new_binary when possible
While enif_make_binary will create heap-binaries from ErlNifBinaries
when possible now, enif_alloc_binary still allocates a Binary* off-heap
which is avoided entirely with enif_make_new_binary.
---
erts/doc/src/erl_nif.xml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/erts/doc/src/erl_nif.xml b/erts/doc/src/erl_nif.xml
index 23f0c66429..9655fcd994 100644
--- a/erts/doc/src/erl_nif.xml
+++ b/erts/doc/src/erl_nif.xml
@@ -974,6 +974,10 @@ typedef struct {
enif_make_binary.
An allocated (and owned) ErlNifBinary can be kept between NIF
calls.
+ If you do not need to reallocate or keep the data alive across NIF
+ calls, consider using
+ enif_make_new_binary instead as it will allocate
+ small binaries on the process heap when possible.
Returns true on success, or false if allocation
fails.
--
cgit v1.2.3