Allow JS_NewArrayBufferWithContents with passed-in free function

RESOLVED WORKSFORME

Status

()

enhancement
RESOLVED WORKSFORME
a year ago
a month ago

People

(Reporter: ptomato, Assigned: ptomato)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

964 bytes, patch
Details | Diff | Splinter Review
20.52 KB, patch
ptomato
: review+
sfink
: feedback+
Details | Diff | Splinter Review
6.61 KB, patch
Details | Diff | Splinter Review
22.01 KB, patch
ptomato
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
JS_NewArrayBufferWithContents() calls js_free() on its data when no longer needed. For embedder purposes, it would be nice to be able to pass in a custom free function so that we can use it with reference-counted data types from external libraries without copying the data.

For example, I'm trying to use it with GLib's GBytes type [1] and in order to do it without a copy and still retain a reference for the embedding's use, the API would need to look something like this:

void
customFree(void *contents, void *userData)
{
    GBytes *data = static_cast<GBytes *>(userData);
    g_assert(g_bytes_get_data(data) == contents);
    g_bytes_unref(data);
}

GBytes *data = ...
JS::RootedObject buf(cx, JS_NewArrayBufferWithContents(cx, g_bytes_get_size(data),
    g_bytes_get_data(data), customFree, data);

I'm aware that ArrayBuffers are difficult objects and that changing them has a lot of ramifications. However, I think this is doable for two reasons: (1) it should not change the behaviour of any code inside Firefox, since the need for working with refcounted data from external libraries is nonexistent; and (2) all that really needs to happen is for ArrayBuffer to call the custom free function instead of js_free(), so it doesn't change any logic.

[1] https://developer.gnome.org/glib/stable/glib-Byte-Arrays.html
(Assignee)

Updated

a year ago
Assignee: nobody → philip.chimento
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
Any suggestions welcome, especially on the StructuredClone part. I don't know what I'm doing there.
Maybe Steve can help you with this.
Flags: needinfo?(sphink)
Comment on attachment 8942472 [details] [diff] [review]
Calculate correct number of slots for ArrayBuffer. r=?

Review of attachment 8942472 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to put the first patch up for review by Waldo (I wrote it.)
Attachment #8942472 - Flags: review?(jwalden+bmo)
Comment on attachment 8942473 [details] [diff] [review]
Allow custom free func for JS_NewArrayBufferWithContents(). r=?

Review of attachment 8942473 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ArrayBufferObject.h
@@ +175,2 @@
>  
> +    static const uint8_t RESERVED_SLOTS = 6;

I'm not really thrilled about bloating up *all* ArrayBuffers, taking away 16 bytes of inline data space for an unused edge case (and bumping up size classes accordingly.)

I think to land this, you'd need to either use a flag to indicate whether it has the additional data and size things accordingly, or make these ArrayBuffers be a subclass (in some sense) that modifies base ArrayBuffer handling as little as possible.

::: js/src/vm/StructuredClone.cpp
@@ +1233,5 @@
>      JSAutoCompartment ac(context(), buffer);
>  
> +    if (buffer->hasCustomFreeFunc()) {
> +        JS_ReportErrorNumberASCII(context(), GetErrorMessage, nullptr,
> +                                  JSMSG_SC_NOT_CLONABLE,

Heh. I guess that's a straightforward way of dealing with the problem. I know other people in the past have wanted refcounted ArrayBuffer data, and they would have needed structured clone to work, but perhaps this is a fine way to add it in incrementally.

I do have some misgivings about creating an ArrayBuffer that doesn't follow the spec like other ArrayBuffers. The spec in question is currently the HTML5 spec, which makes it makes it better, but there are always rumblings about moving structured cloning over to tc39.
(Assignee)

Comment 7

a year ago
> I do have some misgivings about creating an ArrayBuffer that doesn't follow the spec like other ArrayBuffers. The spec in question is currently the HTML5 spec, which makes it makes it better, but there are always rumblings about moving structured cloning over to tc39.

I think it would not be difficult to allow cloning, but that requires adding another slot for the unref function. In the current patch, there's nothing that mandates reference counting -- the customFreeFunc is just that, a function to free the data other than free(). I suppose if the reference counting case would be useful to other people, it would make sense to implement it like that, even if I don't need it.
(Assignee)

Comment 9

a year ago
OK, this patch shows where I'm going with this - adding a new buffer kind, REFCOUNTED, and storing the ref/unref/userData information inline in the fixed slots. (The patch isn't finished yet, I still need to deal with detaching and cloning.)

This does settle on refcounting rather than just a free function. I think it could be made a bit simpler again if it only supported a free function.

I realized also that cloning could always be allowed even without a ref function, just by copying the data when cloning.

Do you have any feedback on this approach before I get too deep into it?
Comment on attachment 8944202 [details] [diff] [review]
Allow reference counted data in JS_NewArrayBufferWithContents()

Review of attachment 8944202 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, I hadn't thought of this option (taking the header out of the inline data space.) I was thinking you'd still store your extra data in slots, as you were before, but only having those slots if you are a ArrayBufferWithFreeFunction or whatever. But this approach works, and it has the advantage that your refUserData can be any 64-bit value; with a slot, you are restricted to what can be stored in a Value (eg a 48-bit pointer aligned to 8 bytes.) The weirdness is that now the data buffer is a mixture of header and data, so anything that accesses or modifies the data pointer needs to be aware of the distinction. I haven't really looked to see whether that's an issue with this patch as is.

But generally, this approach seems good to me.

IIUC, this approach is a superset of your previous one. As in, if you just wanted a buffer with a different free function, you could use a no-op ref and an unconditional free for the deref. Is that true?

With respect to structured cloning, you're right that you could make normal clones just degrade these buffers to regular ArrayBuffers during a clone. That seems to be the right semantics -- if you did anything else, you're liable to end up accidentally making a data racy shared array buffer that can get cloned between threads and corrupt everything. So yes, cloning into a regular ArrayBuffer seems like the right thing for that case, though you can always r? with the current behavior of throwing if you try to clone it, and I'll make up my mind whether to allow it then.

But if you do implement cloning, you might also consider the case of Transferring. For that, I think you'd need to consider the within-thread and the cross-thread (or cross-process) cases separately. Within thread, it seems most useful to increment the ref count and create a new ArrayBuffer holder. Cross-thread, I'm not really sure -- either disallow completely and throw an exception, or detach and copy. ("Detach" means making the origin ArrayBuffer's data inaccessible.

Hm... though I guess I'd want to read the spec[1] again even for the within-thread case. It might say that Transferring always detaches, in which case unreffing and leaving the origin usable wouldn't really be right. In that case, we might want to propose an extension to the spec.

But obviously, we only need to even think about this stuff right now if you wanted to use structured cloning for your own purposes. If not, it would be easiest and best to throw if you attempted to Transfer. (That's what SharedArrayBuffers do [2]).

...and now that I've written that out, I'm thinking that perhaps I'm not thinking straight about what operation would share a ref. Perhaps Transferring should copy, unref, and detach the origin, and regular cloning should copy if cross-thread, ref if same-thread. Then these would basically be Shared(WithinOneThreadOnly)ArrayBuffers or something -- no possibility of data races, but with the nice refcount-based sharing.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm though probably just the spec page it points to.

[2] https://searchfox.org/mozilla-central/source/js/src/vm/StructuredClone.cpp#1109

::: js/src/jsapi.h
@@ +3378,5 @@
>  extern JS_PUBLIC_API(JSObject*)
> +JS_NewArrayBufferWithContents(JSContext* cx, size_t nbytes, void* contents,
> +                              JS::BufferContentsRefFunc ref = nullptr,
> +                              JS::BufferContentsRefFunc unref = nullptr,
> +                              void *refUserData = nullptr);

Small nit -- I would prefer to have a separate API point like JS_NewArrayBufferWithRefcountedContents rather than adding all these new symbols, some disallowed at runtime.
Attachment #8944202 - Flags: feedback+
(Assignee)

Updated

a year ago
Attachment #8942473 - Attachment is obsolete: true
(Assignee)

Comment 11

a year ago
(In reply to Steve Fink [:sfink] [:s:] from comment #10)
> Comment on attachment 8944202 [details] [diff] [review]
> Allow reference counted data in JS_NewArrayBufferWithContents()
> 
> Review of attachment 8944202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oh, I hadn't thought of this option (taking the header out of the inline
> data space.) I was thinking you'd still store your extra data in slots, as
> you were before, but only having those slots if you are a
> ArrayBufferWithFreeFunction or whatever. But this approach works, and it has
> the advantage that your refUserData can be any 64-bit value; with a slot,
> you are restricted to what can be stored in a Value (eg a 48-bit pointer
> aligned to 8 bytes.) The weirdness is that now the data buffer is a mixture
> of header and data, so anything that accesses or modifies the data pointer
> needs to be aware of the distinction. I haven't really looked to see whether
> that's an issue with this patch as is.

In this patch, the data isn't ever stored inline (it's a pointer which comes from the embedding), so only the header is in the inline data space. So I should be able to get away without offsetting the data pointer.

The previous approach with slots had another problem which was that casting a function pointer to a void* in order to store it in a PrivateValue is technically UB since function pointers may be larger than data pointers. I thought that was a good reason to pick this approach instead.

> But generally, this approach seems good to me.

Thanks for the review!

> IIUC, this approach is a superset of your previous one. As in, if you just
> wanted a buffer with a different free function, you could use a no-op ref
> and an unconditional free for the deref. Is that true?

Yes. I am thinking about making that explicit by allowing the ref function to be null.
 
> With respect to structured cloning, you're right that you could make normal
> clones just degrade these buffers to regular ArrayBuffers during a clone.
> That seems to be the right semantics -- if you did anything else, you're
> liable to end up accidentally making a data racy shared array buffer that
> can get cloned between threads and corrupt everything. So yes, cloning into
> a regular ArrayBuffer seems like the right thing for that case, though you
> can always r? with the current behavior of throwing if you try to clone it,
> and I'll make up my mind whether to allow it then.

In fact I realized that cloning ArrayBuffers already copies the data, so I didn't even need to write any code to get copy-on-clone to work.

> But if you do implement cloning, you might also consider the case of
> Transferring. For that, I think you'd need to consider the within-thread and
> the cross-thread (or cross-process) cases separately. Within thread, it
> seems most useful to increment the ref count and create a new ArrayBuffer
> holder. Cross-thread, I'm not really sure -- either disallow completely and
> throw an exception, or detach and copy. ("Detach" means making the origin
> ArrayBuffer's data inaccessible.
> 
> Hm... though I guess I'd want to read the spec[1] again even for the
> within-thread case. It might say that Transferring always detaches, in which
> case unreffing and leaving the origin usable wouldn't really be right. In
> that case, we might want to propose an extension to the spec.
> 
> But obviously, we only need to even think about this stuff right now if you
> wanted to use structured cloning for your own purposes. If not, it would be
> easiest and best to throw if you attempted to Transfer. (That's what
> SharedArrayBuffers do [2]).
>
> ...and now that I've written that out, I'm thinking that perhaps I'm not
> thinking straight about what operation would share a ref. Perhaps
> Transferring should copy, unref, and detach the origin, and regular cloning
> should copy if cross-thread, ref if same-thread. Then these would basically
> be Shared(WithinOneThreadOnly)ArrayBuffers or something -- no possibility of
> data races, but with the nice refcount-based sharing.

I'll need to read up on what all this stuff actually does.

I don't plan to use cloning or transferring for my purposes, so probably the next version of the patch will do as you suggest and throw on transfer, but if it's not hugely difficult to implement then I don't mind taking a look at doing it properly.

It would also be sufficient for my purposes to stipulate that the refcounted data should be immutable, since I'm only planning to pass an immutable data buffer to it. I don't know if that's the case for any of the other potential use cases that you said people had been thinking about?

I do have one follow-up question while I continue to work on the patch: on my try runs I am getting failures from the hazard analysis. For example:

Function 'js::ArrayBufferObject::setNewData(js::FreeOp*, js::ArrayBufferObject::BufferContents, uint32)' has unrooted 'this' of type 'js::ArrayBufferObject*' live across GC call 'js::ArrayBufferObject::releaseData(js::FreeOp*)' at js/src/vm/ArrayBufferObject.cpp:524

I think I need to somehow mark releaseData() as not being able to GC. (That assumes, that the passed-in ref and unref functions may not GC. I hope that's not too restrictive for other use cases, but it's fine for me.) Other ideas are to root 'this' while calling the ref/unref functions. Or is the hazard analysis smart enough to realize what's going on if I surround the calls with AutoCheckCannotGC?
(In reply to Philip Chimento [:ptomato] from comment #11)
> Function 'js::ArrayBufferObject::setNewData(js::FreeOp*,
> js::ArrayBufferObject::BufferContents, uint32)' has unrooted 'this' of type
> 'js::ArrayBufferObject*' live across GC call
> 'js::ArrayBufferObject::releaseData(js::FreeOp*)' at
> js/src/vm/ArrayBufferObject.cpp:524
> 
> I think I need to somehow mark releaseData() as not being able to GC. (That
> assumes, that the passed-in ref and unref functions may not GC. I hope
> that's not too restrictive for other use cases, but it's fine for me.) Other
> ideas are to root 'this' while calling the ref/unref functions. Or is the
> hazard analysis smart enough to realize what's going on if I surround the
> calls with AutoCheckCannotGC?

No, it would get upset with you and say that you promised that you would never GC, then went and called a function that it can't prove won't GC. You could use AutoSuppressGCAnalysis, which would stop the hazard analysis from complaining, yet would still give a runtime assert if you called something from ref/unref that we *know* could potentially GC (eg, allocate a JSString or JSObject). Or given that you're within the engine here, you could use AutoSuppressGC, which would do what it says. But we try to avoid using that as much as possible.

I think your best bet is to start with rooting 'this' as you suggested. It doesn't cost much, and closes off a source of weird errors, however unlikely. But then if it turns out to be a pain (because other code relies on releaseData never GC'ing, so you get a different hazard, and fixing that one exposes a third hazard, etc.), then fall back to AutoSuppressGCAnalysis and add a comment saying the functions passed in cannot GC.
Flags: needinfo?(sphink)
(Assignee)

Comment 13

a year ago
(In reply to Steve Fink [:sfink] [:s:] from comment #12)
> I think your best bet is to start with rooting 'this' as you suggested. It
> doesn't cost much, and closes off a source of weird errors, however
> unlikely. But then if it turns out to be a pain (because other code relies
> on releaseData never GC'ing, so you get a different hazard, and fixing that
> one exposes a third hazard, etc.), then fall back to AutoSuppressGCAnalysis
> and add a comment saying the functions passed in cannot GC.

Unfortunately releaseData() is called from a finalize hook, which means I don't have a context or runtime with which I can root the object. (Or can you get there from the object itself via its compartment? I saw JSCompartment::runtimeFromActiveCooperatingThread() which seemed iffy to call potentially during a GC, and JSCompartment::runtimeFromAnyThread() which basically says "don't use this unless you know what you're doing.") In the absence of that, I'm going to go with AutoSuppressGCAnalysis.

I'll upload a patch later which should be reviewable, in order to make progress towards landing it; I'll try to make cloning/transferring work in a separate patch.
(Assignee)

Comment 15

a year ago
Here's the corresponding try run; the failures all seem to be unrelated and marked 'intermittent' (although I find it hard to tell how I should interpret that!) The hazard jobs are green, though.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13f36b203baa4df9891de39fce5502551ef3dfb2
(Assignee)

Comment 16

a year ago
Attachment #8952065 - Flags: review?(sphink)
Comment on attachment 8945348 [details] [diff] [review]
Allow reference counted data in JS_NewArrayBufferWithContents()

Review of attachment 8945348 [details] [diff] [review]:
-----------------------------------------------------------------

I may regret this, but r=me. If Gecko ends up wanting to use it, I could imagine this changing slightly, but for now this seems useful and properly implemented.

::: js/src/jsapi.h
@@ +3374,5 @@
> + * Create a new array buffer with the given contents. The ref and unref
> + * functions should increment or decrement the reference count of the contents.
> + * These functions allow array buffers to be used with embedder objects that
> + * use reference counting, for example. The contents must not be modified by
> + * any reference holders, internal or external.

Hm. I wonder if this should be tied this closely to refcounting in the API? In API terms, it's more of a "callback buffer" or "externally owned buffer" or something, where it calls the ref/unref functions when handing over ownership or something.

For example, someone could use this to create an ArrayBuffer of static read-only data, and neither function does anything. Or it could be a copy-if-refcount > 1.

The names "ref" and "unref" are fine, since the callbacks are invoked when you create a new reference or drop a reference.

The documentation could still lean heavily on refcounting as an example use.

JS_NewArrayBufferWithRefCallbacks? JS_NewReferencedArrayBuffer? JS_NewExternalArrayBuffer? (That aligns with JS_NewExternalString, though that alignment is problematic -- an external string is described in terms of finalization only, and in fact requires its finalize callback function to be callable from a different thread.)

::: js/src/vm/ArrayBufferObject.cpp
@@ +1193,5 @@
>          if (ownsState == OwnsData) {
> +            if (contents.kind() == REFCOUNTED) {
> +                // Store the RefcountInfo in the inline data slots so that we
> +                // don't use up slots for it in non-refcounted array buffers.
> +                int newSlots = JS_HOWMANY(sizeof(RefcountInfo), sizeof(Value));

This should be a size_t (nslots is a size_t), and I'd prefer 'rcSlots' or 'refCountInfoSlots' or something other than "new".

@@ +1195,5 @@
> +                // Store the RefcountInfo in the inline data slots so that we
> +                // don't use up slots for it in non-refcounted array buffers.
> +                int newSlots = JS_HOWMANY(sizeof(RefcountInfo), sizeof(Value));
> +                MOZ_ASSERT(((void) "RefcountInfo must fit in inline slots",
> +                    reservedSlots + newSlots <= NativeObject::MAX_FIXED_SLOTS));

The arguments here are backwards (which is why you had to cast to void).

  MOZ_ASSERT(condition, "failure message");

::: js/src/vm/ArrayBufferObject.h
@@ +210,5 @@
>  
>          // The dataPointer() is owned by this buffer and should be released
>          // when no longer in use. Releasing the pointer may be done by either
> +        // freeing, releasing a reference, or unmapping it, and how to do this
> +        // is determined by the buffer's other flags.

"either" implies two. Maybe

    ...may be done by freeing, invoking a dereference callback function, or unmapping, as determined by the buffer's other flags.

@@ +257,5 @@
>              MOZ_ASSERT((kind_ & ~KIND_MASK) == 0);
> +            MOZ_ASSERT_IF(ref_ || unref_ || refUserData_, kind_ == REFCOUNTED);
> +            MOZ_ASSERT_IF(kind_ == REFCOUNTED,
> +                          ((void) "Reference counting without unref function doesn't make sense",
> +                           unref_));

MOZ_ASSERT_IF(kind_ == REFCOUNTED, unref_ != nullptr, "Reference...");

@@ +262,5 @@
> +
> +            // BufferContents, since it's only short-lived, doesn't ref or
> +            // unref the data. Therefore, there's no guarantee the data pointer
> +            // is valid. It's the caller's responsibility to make sure the
> +            // BufferContents instance doesn't outlive the data.

I would shorten this to

// BufferContents does not ref or unref the data since it is only short-lived. It is the caller's responsibility to ensure that the BufferContents does not outlive the data.

Though I also wonder whether "short-lived" is really the relevant property. Hypothetically, if you *did* call ref()/unref() for BufferContents, it doesn't feel like just a performance problem. It seems semantically wrong. Maybe it's because it is a detail of the internal implementation; we don't want user-visible behavior differences resulting from its use. (But from the point of view of this code, the comment about the caller is totally valid; the comment is for the internal users.)

Maybe "...since it is internal and short-lived" is enough.

@@ +284,5 @@
> +                                               JS::BufferContentsRefFunc unref,
> +                                               void* refUserData = nullptr)
> +        {
> +            return BufferContents(static_cast<uint8_t*>(data), REFCOUNTED,
> +                                  ref, unref, refUserData);

We wrap code at 99 characters -- this should fit on one line? (Comments wrap at 79 chars.)

@@ +391,5 @@
>      BufferContents contents() const {
> +        if (isRefcounted())
> +            return BufferContents(dataPointer(), REFCOUNTED,
> +                                  refcountInfo()->ref, refcountInfo()->unref,
> +                                  refcountInfo()->refUserData);

When a statement is broken over multiple lines like this, bracket with {} even if it's a single statement.

::: js/src/vm/StructuredClone.cpp
@@ +1114,5 @@
> +        // Refcounted array buffers may be able to be transferred in the future,
> +        // but that is not currently implemented.
> +
> +        if (tObj->is<ArrayBufferObject>() && tObj->as<ArrayBufferObject>().isRefcounted())
> +            return reportDataCloneError(JS_SCERR_UNSUPPORTED_TYPE);

I think JS_SCERR_TRANSFERABLE is more appropriate here. (That's what you would get if you tried to transfer a number or a string or something, or if an embedder-defined callback rejected the object you gave it.)
Attachment #8945348 - Flags: review?(sphink) → review+
Attachment #8944202 - Attachment is obsolete: true
Comment on attachment 8952065 [details] [diff] [review]
Implement cloning for refcounted array buffers

Review of attachment 8952065 [details] [diff] [review]:
-----------------------------------------------------------------

I will publish this review in its incomplete state. I'm still unsure of the desired semantics here. What does it mean to Transfer a refcounted ArrayBuffer? This implementation does a useful thing, which is to give the recipient a pointer to a now-shared data buffer. But spec-wise, Transferring is also supposed to "detach" the original ArrayBuffer. And SharedArrayBuffer creates shared buffers via normal clones (and forbids Transferring entirely).

Again, considering that these are embedder-only for now and the embedders (i.e., you and you alone for them moment) currently want the effect of sharing a reference and have no use for Transferring, I'm inclined to say that for now at least we shouldn't allow Transferring at all.

But jorendorff has much better taste than me, so I'm going to punt the review over to him to see what he thinks. (He may also have an issue with the original patch, given that he's working on rationalizing and simplifying things.)

::: js/src/vm/StructuredClone.cpp
@@ +1240,5 @@
>      Rooted<ArrayBufferObject*> buffer(context(), &CheckedUnwrap(obj)->as<ArrayBufferObject>());
>      JSAutoCompartment ac(context(), buffer);
>  
> +    if (buffer->isRefcounted() &&
> +        scope == JS::StructuredCloneScope::SameProcessSameThread) {

nit: { on its own line, or put it all on one line.

@@ +1252,5 @@
> +            // array buffer is destroyed before the data is read out.
> +            contents.refFunc()(contents.data(), contents.refUserData());
> +
> +            return out.writePair(SCTAG_REFCOUNTED_ARRAY_BUFFER_OBJECT,
> +                    buffer->byteLength()) &&

99-char lines; can fit this first writePair() on a single line.

@@ +2054,5 @@
> +    if (!in.readPtr(&dataPtr) ||
> +        !in.readPtr(&refPtr) ||
> +        !in.readPtr(&unrefPtr) ||
> +        !in.readPtr(&refUserData))
> +        return in.reportTruncated();

bracket {} the return statement since the condition is multiline.
Attachment #8952065 - Flags: review?(sphink) → review?(jorendorff)
Also needinfo? baku to see if he has a use for refcounted ArrayBuffers, to see if we have other use cases to consider.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 20

a year ago
Thanks for the reviews. I'll try to revise this over the next few days, but I'll respond here to points that I don't need to write code for.

Of the name suggestions, I like JS_NewExternalArrayBuffer best, but that seems potentially confusing with JS_NewArrayBufferWithExternalContents ... which is implemented differently (PLAIN buffer, DoesntOwnData vs. REFCOUNTED buffer, OwnsData) but now that I think about it essentially serves the same purpose as passing nullptr for both callbacks.

With the assertions - I was actually going for the comma operator because this is how I cram an assertion message in my code elsewhere :-) I had forgotten about the second argument to MOZ_ASSERT, I'll use it!

> Again, considering that these are embedder-only for now and the embedders (i.e., you and you alone for them moment) currently want the effect of sharing a reference and have no use for Transferring, I'm inclined to say that for now at least we shouldn't allow Transferring at all.

That sounds fine, since I still haven't gotten Transferring to work, and was dreading having to go back to the spec to figure it out. This second patch only implements JS_StructuredClone with the behaviour you outlined previously (ref if same-thread, copy otherwise), and still throws when attempting to Transfer. But that's correct, I don't need anything having to do with structured cloning for my purposes. I'll still revise the patch according to the comments you had so far.
(Assignee)

Updated

a year ago
Attachment #8945348 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8952065 - Attachment is obsolete: true
Attachment #8952065 - Flags: review?(jorendorff)
(Assignee)

Updated

a year ago
Attachment #8953704 - Flags: review+
Attachment #8953704 - Flags: feedback?(sphink)
(Assignee)

Updated

a year ago
Attachment #8953705 - Flags: review?(jorendorff)
(Assignee)

Comment 23

a year ago
Updated patches; I've carried forward the r+ on the first one, but @sfink I've requested feedback from you on the new name in the API. I went with JS_NewExternalArrayBuffer(), but I'm fine to change it to something else.

> For example, someone could use this to create an ArrayBuffer of static read-only data, and neither function does anything.

That was disallowed in the previous patch, but why not? I've added a test for this case. Note that JS_NewArrayBufferWithExternalContents() implements this differently.

It seems like we could maybe fold that API into this one, and create a buffer that DoesntOwnData in the case where both the ref and unref callbacks are null... or just remove it? It looks like JS_NewArrayBufferWithExternalContents() isn't used in Gecko. (https://searchfox.org/mozilla-central/search?q=JS_NewArrayBufferWithExternalContents&case=false&regexp=false&path=) Maybe there are other embedders using it.

> Or it could be a copy-if-refcount > 1.

I don't think that's possible with this implementation, because the ref function would need to return a new data pointer and the array buffer would update itself to point to the ref function's return value.
Comment on attachment 8953704 [details] [diff] [review]
Allow reference counted data in JS_NewArrayBufferWithContents()

Review of attachment 8953704 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with the name. Since the other function isn't being used, I'd prefer folding these together as you mentioned, but that can be a later change.

Doesn't this still require the other patch to be reviewed before this can be checked in?
Attachment #8953704 - Flags: feedback?(sphink) → feedback+
Comment on attachment 8953704 [details] [diff] [review]
Allow reference counted data in JS_NewArrayBufferWithContents()

(as per IRC, the other patch is no longer needed; this no longer uses slots.)
Attachment #8953704 - Flags: checkin?

Comment 26

a year ago
Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8720aef9b3a5
Allow reference counted data in JS_NewArrayBufferWithContents(). r=sfink

Comment 27

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8720aef9b3a5
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Assignee)

Comment 28

a year ago
The other patches are still awaiting review, so I'll reopen this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 29

a year ago
- Fold the existing JS_NewArrayBufferWithExternalContents() API into the
  new JS_NewExternalArrayBuffer() API.
- Fold testExternalArrayBuffer.cpp into testArrayBuffer.cpp, as it already
  tested mostly the same things.
- Apply an optimization: if neither ref nor unref func are given, then set
  DoesntOwnData on the buffer.
Attachment #8959867 - Flags: review?(sphink)
(Assignee)

Comment 30

a year ago
This new patch, hopefully the last one needed for this bug, is as far as I know independent of the other two pending patches. It gets rid of the now-obsolete (and only used in tests) API JS_NewArrayBufferWithExternalContents(). It's equivalent to JS_NewExternalArrayBuffer(..., nullptr, nullptr).
(Sorry for the delay, I hope to get to reviewing this soon.)
Comment on attachment 8953705 [details] [diff] [review]
Implement cloning for refcounted array buffers

Review of attachment 8953705 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review. I'm OK with creating ArrayBuffers that are backed by external data, but I'm not convinced reference counting is a good idea.

It's definitely a stretch as far as spec compatibility is concerned. More importantly, we may someday add JIT compiler optimizations that assume that ArrayBuffers never alias one another. I don't want to create a situation where implementing an optimization would mysteriously break some embedders.

For your use case, how important is it for cloning to behave exactly this way? Suppose we did not implement reference counting but just had a destructor. Cloning would transfer the buffer (or if you don't pass it as a transferable, copy the buffer contents into a new malloc buffer). Would that be sufficient? If not, I suggest creating a different class and using JSStructuredCloneCallbacks.
Attachment #8953705 - Flags: review?(jorendorff)
However comment 32 gets resolved, the jsapi.h comment should be unmistakable about some things:

Lifetime safety: The caller must keep the buffer alive until `unref()` is called; or, if `unref` is null, until the JSRuntime is destroyed.

Thread safety: The caller must not access the buffer on other threads. The JS engine will not allow the buffer to be transferred to other threads. If you try to transfer an external ArrayBuffer to another thread, the data is copied to a new malloc buffer. `unref()` will be called only on the current thread. (Note: I didn't check that this is really true, as implemented. Classes can opt in to be background-finalized, from another thread; but I didn't look to see if ArrayBuffer opts in.)
Ugh, jorendorff is totally correct.

ArrayBuffers are background-finalizable, which means that unref() will certainly be called from a different thread. And to change that, you'd need to either (1) create a separate JS::Class for them, which would probably mean changing a bunch of code to handle that class in addition to ArrayBufferObject::class_; or (2) rather than calling unref() in the finalizer, make the finalizer insert into to a thread-safe queue (or other collection) and invoke unref() from the main thread at the end of the GC. But (2) would only fix the unref problem; the aliasing problem would remain, and probably the best way to handle that is with (1) because a separate JS::Class could indicate whether data is aliasable or not. But then all of your views (typed arrays or whatever) would need to handle both cases, or perhaps you might need to split out separate Classes for all of *them*... ugh.
(Assignee)

Comment 35

a year ago
Thanks for the review. I'm fine with discarding this cloning patch. The other patch that was already committed, is enough for my use case. I wanted to implement cloning for completeness, but it sounds like we can consider it complete without cloning, really.

For my use case, it's OK to call unref() from a different thread, as well. So as far as I'm concerned there's no need to split out separate JS::Classes either. The data buffers I'm using are immutable and threadsafe, so I'm OK with imposing that restriction on any data buffers passed in to JS_NewExternalArrayBuffer(). I realize that might make this API less useful for others, though.

It's been a while since I looked at the patch, but I *think* the already-committed patch doesn't alias ArrayBuffers. The ref function is only called once when assuming the initial reference to the data. If we never want to make use of the ref function, since the only other thing I can think of to do with it is aliasing, we may as well remove it and let the caller of JS_NewExternalArrayBuffer() ref the data before calling if they need to. (jorendorff, I think this is what you mean by "just having a destructor"?)

What would you prefer?

- Change nothing beyond what was already committed
- Remove the ref argument from JS_NewExternalArrayBuffer()

In either case I will make another patch with the requested clarification to the comment in jsapi.h.
Sorry I didn't see this, ptomato. Yes, please remove the `ref` function. That's exactly what I meant.

Thanks for helping with the comment, too. Remember, the "thread safety" part of my comment 33 is inaccurate, as sfink pointed out in comment 34.

Please also land something like the jsapi-tests in the cloning patch, at least. We need to test that (a) transferring an external ArrayBuffer, using SameProcess, either throws or works; (b) cloning an external ArrayBuffer, using DifferentProcess, either throws or makes a normal ArrayBuffer with a copy of the same bytes. Throwing is a bit lame, but I mainly care about not crashing (or aliasing, calling unref twice, etc.)
Flags: needinfo?(philip.chimento)
(Assignee)

Updated

a year ago
Attachment #8953705 - Attachment is obsolete: true
(Assignee)

Comment 37

a year ago
We actually don't want to allow aliased array buffers at all, so this
removes the ref argument from JS_NewExternalArrayBuffer() and renames
"unref" to "free".

Adjusts some comments, and also adds tests to make sure buffers are not
aliased when cloning.
Attachment #8973004 - Flags: review?(jorendorff)
(Assignee)

Comment 38

a year ago
- Fold the existing JS_NewArrayBufferWithExternalContents() API into the
  new JS_NewExternalArrayBuffer() API.
- Fold testExternalArrayBuffer.cpp into testArrayBuffer.cpp, as it already
  tested mostly the same things.
- Apply an optimization: if neither ref nor unref func are given, then set
  DoesntOwnData on the buffer.
Attachment #8973005 - Flags: review?(sphink)
(Assignee)

Updated

a year ago
Attachment #8959867 - Attachment is obsolete: true
Attachment #8959867 - Flags: review?(sphink)
(Assignee)

Comment 39

a year ago
Here are the new patches!

The only thing I'm not sure about is whether the SameProcess cloning test that I put in there, covers the case that you mentioned: "transferring an external ArrayBuffer, using SameProcess"
Flags: needinfo?(philip.chimento)
Comment on attachment 8973004 [details] [diff] [review]
Remove ref argument from JS_NewExternalArrayBuffer()

Review of attachment 8973004 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good patch. Thanks!

::: js/src/jsapi-tests/testArrayBuffer.cpp
@@ +229,4 @@
>  BEGIN_TEST(testArrayBuffer_customFreeFunc)
>  {
> +    ExternalData data("One two three four");
> +    // The buffer takes ownership of the data.

Style nit: Always put a blank line before a comment that takes up a whole line or more. (Except if the comment is at the beginning of a block or file.)

@@ +254,5 @@
>  
>  BEGIN_TEST(testArrayBuffer_staticContents)
>  {
> +    ExternalData data("One two three four");
> +    // When not passing a free function, the buffer doesn't own the data.

And here.

::: js/src/jsapi-tests/testStructuredClone.cpp
@@ +144,5 @@
> +    // SameProcessSameThread is tested above.
> +    return true;
> +}
> +
> +bool TestStructuredCloneCopy(JS::StructuredCloneScope scope)

Style nit: method names should start with lowercase letters: testStructuredCloneCopy.

@@ +182,5 @@
> +
> +    return true;
> +}
> +
> +bool Clone(JS::StructuredCloneScope scope, JS::HandleValue v1, JS::MutableHandleValue v2)

Same here.
Attachment #8973004 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 41

11 months ago
We actually don't want to allow aliased array buffers at all, so this
removes the ref argument from JS_NewExternalArrayBuffer() and renames
"unref" to "free".

Adjusts some comments, and also adds tests to make sure buffers are not
aliased when cloning.
(Assignee)

Updated

11 months ago
Attachment #8973004 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8975505 - Flags: review+
(Assignee)

Comment 42

11 months ago
OK, patch updated. (The checkin request applies only to attachment 8975505 [details] [diff] [review].)

Jorendorff: should I request uplift of this patch to ESR60 or should we leave ESR60 with the ref argument?

Sfink, Waldo: The remaining two patches aren't strictly necessary for my purposes, but I'd appreciate it if you could review them so we can close this bug!
Flags: needinfo?(sphink)
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jorendorff)
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Target Milestone: mozilla60 → ---

Comment 43

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0542781fe387
Remove ref argument from JS_NewExternalArrayBuffer(). r=jorendorff
Keywords: checkin-needed
Keywords: leave-open
Flags: needinfo?(jorendorff)
Comment on attachment 8942472 [details] [diff] [review]
Calculate correct number of slots for ArrayBuffer. r=?

Review of attachment 8942472 [details] [diff] [review]:
-----------------------------------------------------------------

I...think this is entirely obsolete at this point?  We manually reserved extra space for `FreeInfo` iff we need it now, without adding a special case to this generalized code or bumping `ABO::RESERVED_SLOTS`.  Canceling, and if some shred of this is still usable after all of bug 15292298 has landed, please post a new patch.
Attachment #8942472 - Flags: review?(jwalden)
(Assignee)

Comment 46

2 months ago

It's been over a year so I can't remember the details but sfink wrote that patch because there was some internal inconsistency in ABO. I guess if it's being refactored anyway, then it's OK to drop that patch.

Flags: needinfo?(jwalden)
Comment on attachment 8973005 [details] [diff] [review]
Move JS_NewArrayBufferWithExternalContents() into JS_NewExternalArrayBuffer()

Review of attachment 8973005 [details] [diff] [review]:
-----------------------------------------------------------------

Looks obsolete now to me.
Attachment #8973005 - Flags: review?(sphink)

It's unclear to me if there's anything left here to work on, after the recent restructuring that Waldo did. I suspect we should probably close out this bug at this point, and if things remain, to open new bugs to work on them on top of the revised situation.

I'm guessing you might be carrying this patch on your own copy, though, so this may require some repeated work?

Flags: needinfo?(sphink)
(Assignee)

Comment 49

2 months ago

I'm not carrying this patch, so no worries :-) It was only to get rid of an API wart. The rationale for it was:

This new patch, hopefully the last one needed for this bug, is as far as I know independent of the other two pending patches. It gets rid of the now-obsolete (and only used in tests) API JS_NewArrayBufferWithExternalContents(). It's equivalent to JS_NewExternalArrayBuffer(..., nullptr, nullptr).

Since Waldo's refactor, JS_NewArrayBufferWithExternalContents() is now called JS_NewArrayBufferWithUserOwnedContents() and is no longer only used in tests. So it's probably fine to leave as-is. I am not sure whether it's still equivalent to JS_NewExternalArrayBuffer(..., nullptr, nullptr).

Thanks, Philip.

Status: REOPENED → RESOLVED
Last Resolved: a year agoa month ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.