Make ArrayBuffer's handling of kinds and data-ownership sane and simple and understandable
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(39 files)
The complexity has sprawled. I fear to review or touch the code because I can't understand it enough to safely touch it. Bug 1502562 was the last straw, where like a five-line patch I felt safer/more confident not taking and leaking, than taking and not leaking but maybe terrible things happen too?
So, have a gonzo patch stack of mostly a zillion smallish transformation, only a few of which actually change how the world functions, and in the end we have an understandable system.
sfink has volunteered as tribute. Or been volunteered; I forget which.
|
Assignee | |
Comment 1•5 years ago
|
||
|
|
Assignee | |
Comment 2•5 years ago
|
||
This becomes clearer in what prepareForAsmJS does and doesn't mark, at some distant point in this patch stack, but for now a comment on this seems helpful.
|
|
Assignee | |
Comment 3•5 years ago
|
||
I'm not sure I like the idea of BufferContents as a thing to signal failure, but it exists, we do have places that need to return a data pointer and one of a few different kinds, so it's not absolutely terrible. Meanwhile, having a thing for creating failure instances of it seems useful for calling out that possibility.
|
|
Assignee | |
Comment 4•5 years ago
|
||
So the problem with the existing kinds is they act at cross purposes with OwnsData/DoesntOwnData. Really we want kind to identify ownership. Splitting PLAIN up a few obvious ways is a step in that direction.
|
|
Assignee | |
Comment 5•5 years ago
|
||
|
|
Assignee | |
Comment 6•5 years ago
|
||
Obviously inline data is either never "owned", or always "owned". So it having its own kind is a no-brainer.
|
|
Assignee | |
Comment 7•5 years ago
|
||
|
|
Assignee | |
Comment 8•5 years ago
|
||
jorendorff is thumbs-down on this API: https://mozilla.logbot.info/jsapi/20190213#c15962840 Ix-nay.
|
|
Assignee | |
Comment 9•5 years ago
|
||
This made sense when the prototype was an ArrayBuffer. It isn't now. This doesn't make sense now.
|
|
Assignee | |
Comment 10•5 years ago
|
||
|
|
Assignee | |
Comment 11•5 years ago
|
||
Detritus from when there was just one function that was a floor wax and a dessert topping and a Swiss army knife and a footgun.
|
|
Assignee | |
Comment 12•5 years ago
|
||
I tried reading through the callocCanGC implementation earlier, and I would have sworn to it reporting OOM on failure. But it apparently does not. We could rathole on whether OOM failure should just stop execution in some of these callers as this patch makes it, or fail-without-OOM handled more gracefully is better. I argue this excess complexity does not pay off to have to think about. Moreover, at the end of all the patches here, the only two callers of AABC basically are in functions that really should have OOM reporting by default, so this is the way to go.
|
|
Assignee | |
Comment 13•5 years ago
|
||
|
|
Assignee | |
Comment 14•5 years ago
|
||
|
|
Assignee | |
Comment 15•5 years ago
|
||
|
|
Assignee | |
Comment 16•5 years ago
|
||
|
|
Assignee | |
Comment 17•5 years ago
|
||
|
|
Assignee | |
Comment 18•5 years ago
|
||
So NO_DATA will ultimately get used in two places: * for ArrayBuffers that become detached * for wasm ArrayBuffers created by a memory.grow operation, whose contents are then overwritten by an original buffer's data after successful size increase Rather than overload malloc'd data and allow such data to be null, or making these users be INLINE_DATA at cost of more onerous initialization, just add another kind for null data. Easy. Simple. |ownsData()| and all related code will be very gradually phased out as this patch stack proceeds; I'm not sure just how much I made NO_DATA here truly owns-agnostic. Don't sweat it too hard when reviewing, it all comes together in the end.
|
|
Assignee | |
Comment 19•5 years ago
|
||
|
|
Assignee | |
Comment 20•5 years ago
|
||
This function is a huge part of the horror of the current code. That, and the |bool hasStealableContents| argument to the |stealContents| function. The replacements here do not necessarily make things as absolutely clearer as you'd want. But it's a step in the right direction. Small steps in small patches.
|
|
Assignee | |
Comment 21•5 years ago
|
||
Okay, this starts to get a little tricky, sort of. The ultimate aim is for |ownsData()| and the OwnsState enum and the flag-bit to all disappear. I achieve this end by gradually making everything OwnsData. Once everything is OwnsData, anything that queries that is an opportunity to eliminate dead code. *Sometimes* the approach for this is to handle a kind identically regardless of ownership. For example, releaseData is gradually changed to do this. Other places, you have to fill in some fallback code in the case where |ownsData()| now is true for more-exotic kinds that we could just skate past before. And further, the code that *creates* these ArrayBuffers for these contents, needs to deal with |ownsData()| no longer being the right test necessarily. I tried to audit every user of ownsData to deal with this stuff. There isn't a great way to represent that/make that verifiable in a patch. The XXX breadcrumbs with explanations are the best I could do, I think. (By end of patch stack they've all been removed.)
|
|
Assignee | |
Comment 22•5 years ago
|
||
[cx] versus [&cx] is pretty eh in my book. At this point now I don't even remember which I picked.
|
|
Assignee | |
Comment 23•5 years ago
|
||
Channeling three separate ideas through the same function with the caller passing a boolean to direct the particular mode of operation in a very finicky way is the worst thing in the world. Let's undo that one caller at a time.
|
|
Assignee | |
Comment 24•5 years ago
|
||
This alternative is so much simpler than using ABO::stealContents it's not even funny.
|
|
Assignee | |
Comment 25•5 years ago
|
||
I'm not sure the "replace contents by nulling them out in a way that doesn't release, then detach using the exact same contents" is aesthetically pleasing or best. It gets the job done far more clearly than the current thing. I would be open to suggestions for improving on it once all this is landed, for sure. (Might be I'll even come up with those improvements, myself. But I've spent about a week and a half on all this to be able to process about a single five-line review, so I am a little un-energized now.)
|
|
Assignee | |
Comment 26•5 years ago
|
||
Okay, from here on it's all clear sailing -- ABO::stealContents was the last user of DoesntOwnData! Just a stream of a bunch of patches incrementally pruning dead code, inlining arguments constant across all callers, etc.
|
|
Assignee | |
Comment 27•5 years ago
|
||
|
|
Assignee | |
Comment 28•5 years ago
|
||
|
|
Assignee | |
Comment 29•5 years ago
|
||
|
|
Assignee | |
Comment 30•5 years ago
|
||
Could have done this in the previous patch, figured better to keep changes minimal-er.
|
|
Assignee | |
Comment 31•5 years ago
|
||
|
|
Assignee | |
Comment 32•5 years ago
|
||
|
|
Assignee | |
Comment 33•5 years ago
|
||
|
|
Assignee | |
Comment 35•5 years ago
|
||
...oh hey, these functions are not actually used any more after a point in this stack, I could have removed them earlier.
|
|
Assignee | |
Comment 36•5 years ago
|
||
This complexity was always a very terrible thing. :-( Glad to see it die.
|
|
Assignee | |
Comment 37•5 years ago
|
||
|
|
Assignee | |
Comment 38•5 years ago
|
||
And that's it! ("that") Hopefully I kept everything small enough this should be mostly simple to run through begin to end.
I can also upload ABO.{cpp,h} and StructuredClone.cpp if the full final states of those files ends up helpful for understanding where things end up, if you want.
|
||
Comment 39•5 years ago
|
||
Comment on attachment 9045266 [details] [diff] [review] Add an extra bit to ArrayBufferObject::BufferKind's KIND_MASK to permit adding additional buffer kinds beyond the current four Review of attachment 9045266 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.h @@ +177,5 @@ > enum BufferKind { > + PLAIN = 0b000, // malloced or inline data > + WASM = 0b001, > + MAPPED = 0b010, > + EXTERNAL = 0b011, I really enjoy our new style constraints, don't you? @@ +205,5 @@ > // Array buffers which do not own their data include buffers that > // allocate their data inline, and buffers that are created lazily for > // typed objects with inline storage, in which case the buffer points > // directly to the typed object's storage. > + OWNS_DATA = 0b1'0000, Whoa. 0b? ' in numeric literals? What is this world coming to?
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Comment 40•5 years ago
|
||
Comment on attachment 9045277 [details] [diff] [review] Split PLAIN into PLAIN_DATA and USER_OWNED ArrayBuffer data types to clearly segregate the two, rather than categorizing them both as the same thing Review of attachment 9045277 [details] [diff] [review]: ----------------------------------------------------------------- A truly righteous change, thank you. ::: js/src/shell/js.cpp @@ +1915,5 @@ > static bool CacheEntry_setBytecode(JSContext* cx, HandleObject cache, > uint8_t* buffer, uint32_t length) { > MOZ_ASSERT(CacheEntry_isCacheEntry(cache)); > > + using BufferContents = ArrayBufferObject::BufferContents; `using ArrayBufferObject::BufferContents;` doesn't work? ::: js/src/vm/ArrayBufferObject.cpp @@ +1801,5 @@ > + // The caller assumes that a plain malloc'd buffer is returned. To steal > + // actual contents, then, we must have |hasStealableContents()| *and* the > + // contents must be |isPlainData()|. (Mapped data would not be malloc'd; > + // user-provided data we flat-out know nothing about at all -- although it > + // *should* have not passed the |hasStealableContents()| check anyway.) Ugh. This stuff all sucks. Hopefully this patch stack will make it tolerable? ::: js/src/vm/ArrayBufferObject.h @@ +179,5 @@ > + PLAIN_DATA = 0b000, > + > + /** > + * User-owned memory. The associated buffer must be manually detached > + * before the user-owned memory becomes invalid. On first read, this sounded very wrong -- it sounded to me like you should manually detach in order to correctly invalidate the memory, which doesn't make sense and confused me. I just automatically read it as "this is the correct series of events: (1) detach, (2) the memory becomes invalid." Uh... "The owner must not allow the memory to become invalid without manually detaching it first."? "The owner must keep the memory valid until the associated buffer is detached."? I don't know.
|
|
Assignee | |
Comment 41•5 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #40)
- using BufferContents = ArrayBufferObject::BufferContents;
using ArrayBufferObject::BufferContents;doesn't work?
That's the syntax for accessing inherited members (using declarations), not for declaring type aliases. I'm not sure why C++ doesn't let you use both when there's not ambiguity or something.
Ugh. This stuff all sucks. Hopefully this patch stack will make it tolerable?
Maybe you think it be like it is, and it do.
- /**
* User-owned memory. The associated buffer must be manually detached* before the user-owned memory becomes invalid.On first read, this sounded very wrong -- it sounded to me like you should
manually detach in order to correctly invalidate the memory, which doesn't
make sense and confused me. I just automatically read it as "this is the
correct series of events: (1) detach, (2) the memory becomes invalid."Uh... "The owner must not allow the memory to become invalid without
manually detaching it first."? "The owner must keep the memory valid until
the associated buffer is detached."? I don't know.
Some patch somewhere, maybe this bug for all I remember, adds docs by JS_NewArrayBufferWithExternalContents (or its renaming, can't remember where that is either) that says what you have to do when. IMO.
|
|
Assignee | |
Comment 42•5 years ago
|
||
Try-run is surprisingly clean, only four failures in the whole of it without even categorizing anything or anything.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e44562b5cdadea69fec19c362812ad5258bcfd37
|
||
Comment 43•5 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9063e76442 Add an extra bit to ArrayBufferObject::BufferKind's KIND_MASK to permit adding additional buffer kinds beyond the current four. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/f24c3019443f Change the comment by ArrayBufferFlags's FOR_ASMJS to indicate that buffers prepared for asm.js must be PLAIN, MAPPED, or EXTERNAL. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/6e25dc78ef1e Add a BufferContents::createFailed() to use in situations where a failure occurs, rather than overloading createPlain(nullptr). r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/2849a2838ea5 Split PLAIN into PLAIN_DATA and USER_OWNED ArrayBuffer data types to clearly segregate the two, rather than categorizing them both as the same thing. r=sfink
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 44•5 years ago
|
||
Obviously it would be better if we did more exhaustive switchin', less ad hoc series of if-tests. Strangely, this is more or less the only place that can be so improved: without entirely trying I have switches in the obvious places at this point. But if you happen to see more places that could benefit from this, feel free to note 'em.
|
|
||
Updated•5 years ago
|
|
|
||
Comment 45•5 years ago
|
||
Comment on attachment 9045279 [details] [diff] [review] Split PLAIN into INLINE_DATA/MALLOCED for ArrayBuffer kinds Review of attachment 9045279 [details] [diff] [review]: ----------------------------------------------------------------- \o/
|
||
Comment 46•5 years ago
|
||
| bugherder | ||
|
|
||
Comment 47•5 years ago
|
||
Comment on attachment 9045281 [details] [diff] [review] Move most of JS_ExternalizeArrayBufferContents into a static member function on ArrayBufferObject so that internals-observing code isn't smeared across two functions Review of attachment 9045281 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.h @@ +338,5 @@ > > static size_t objectMoved(JSObject* obj, JSObject* old); > > + // Convert this ArrayBuffer's storage to memory allocated using the standard > + // SpiderMonkey allocator (this may be a no-op), then return pointer to that bcdefghijklmnopqrstuvwxyz (though if this functionality is going to die later in this series, then never mind.)
|
|
||
Comment 48•5 years ago
|
||
Comment on attachment 9045282 [details] [diff] [review] Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity Review of attachment 9045282 [details] [diff] [review]: ----------------------------------------------------------------- ...that didn't take long.
|
|
||
Updated•5 years ago
|
|
|
||
Comment 49•5 years ago
|
||
Comment on attachment 9045284 [details] [diff] [review] Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function Review of attachment 9045284 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +1322,5 @@ > + } > + return nullptr; > + } > + > + MOZ_ASSERT(!gc::IsInsideNursery(buffer)); How does this work? The object is allocated with GenericObject, not TenuredObject, but then is asserted here. I don't see why GetGCObjectKind(nslots) would enforce a non-nursery object? At the very least, it seems highly confusing to use GenericObject but assert tenured. ::: js/src/vm/TypedArrayObject.cpp @@ +78,5 @@ > } > > AutoRealm ar(cx, tarray); > Rooted<ArrayBufferObject*> buffer( > + cx, ArrayBufferObject::createZeroed(cx, tarray->byteLength())); It would be nice to have those stolen 19 columns returned to us.
|
|
||
Comment 50•5 years ago
|
||
Comment on attachment 9045285 [details] [diff] [review] Remove trailing arguments to ArrayBufferObject::createForContents that are identical for every caller Review of attachment 9045285 [details] [diff] [review]: ----------------------------------------------------------------- Nice how the default was the opposite of what was used.
|
|
||
Comment 51•5 years ago
|
||
Comment on attachment 9045286 [details] [diff] [review] Make AllocateArrayBufferContents return uint8_t*, and make its callers consistently not redundantly report OOMs Review of attachment 9045286 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +442,5 @@ > return true; > } > > +static uint8_t* AllocateArrayBufferContents(JSContext* cx, uint32_t nbytes) { > + auto* p = cx->pod_callocCanGC<uint8_t>(nbytes, js::ArrayBufferContentsArena); There's really no way to remember whether this reports or not. I wonder if we should have a mandatory ReportOOM/NoReportOOM flag or something.
|
|
||
Updated•5 years ago
|
|
|
||
Comment 52•5 years ago
|
||
Comment on attachment 9045288 [details] [diff] [review] Simplify ArrayBufferObject::createForContents some now that it's only ever passed non-null contents Review of attachment 9045288 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +1236,5 @@ > } > > + // ...curious why "mapped" bytes are being fed into a "malloc" counter? > + // Me too. ¯\_(ツ)_/¯ > + cx->updateMallocCounter(nAllocated); Bug 1037358, some guy named Waldo reviewed it a mere 5 years ago. Basically, we were running out of address space. So we're kind of cheating and using the malloc counter for both virtual and physical memory usage. Probably worth a comment.
|
|
||
Comment 53•5 years ago
|
||
Comment on attachment 9045289 [details] [diff] [review] Consistently use "ArrayBuffer" instead of the vaguer "array buffer" in ArrayBuffer-related JSAPI descriptions Review of attachment 9045289 [details] [diff] [review]: ----------------------------------------------------------------- Fair complaint. ::: js/src/jsfriendapi.h @@ +1853,1 @@ > * typed array view. The spec doesn't have some weirdo TypedArrayView semantic-only base class?
|
|
||
Comment 54•5 years ago
|
||
Comment on attachment 9045290 [details] [diff] [review] Add BufferContents::createFoo functions for every kind so that the templaty, harder-to-search-for create function can be removed Review of attachment 9045290 [details] [diff] [review]: ----------------------------------------------------------------- I still kinda regret the "create" terminology too. It's not really creating anything of interest, just bundling up a pointer with a descriptor. Oh well.
|
|
||
Updated•5 years ago
|
|
|
||
Comment 55•5 years ago
|
||
Comment on attachment 9045295 [details] [diff] [review] Add BufferKind::NO_DATA for ArrayBuffers that have no data (whether because byteLength is zero or because the ArrayBuffer is detached), for which the value of |ownsData()| is irrelevant Review of attachment 9045295 [details] [diff] [review]: ----------------------------------------------------------------- Whatever. Just make it go away. ::: js/src/vm/ArrayBufferObject.cpp @@ +1814,5 @@ > + // and would signal failure if we returned it, plus |hasStealableContents()| > + // specifically excludes it; user-provided data we know nothing about at all > + // -- although it *should* have not passed the |hasStealableContents()| check > + // anyway because it's not owned; mapped data wouldn't be malloc'd; external > + // data has to be freed using a provided function.) A perfect description about how this code jumped the shark at some point. ::: js/src/vm/ArrayBufferObject.h @@ +272,5 @@ > return BufferContents(static_cast<uint8_t*>(data), MALLOCED); > } > > + static BufferContents createNoData() { > + return BufferContents(nullptr, NO_DATA); ♫ Yes, we have no bananas. We have no bananas today! ♬
|
|
||
Updated•5 years ago
|
|
|
||
Comment 56•5 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a391fd808f Rename JS_NewArrayBufferWithExternalContents to JS_NewArrayBufferWithUserOwnedContents to better accord with the USER_OWNED ArrayBufferKind name, and to avoid namespace overload with the semantically distinct JS_NewExternalArrayBuffer and JS_ExternalizeArrayBufferContents functions. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/9294b0d54597 Split PLAIN into INLINE_DATA/MALLOCED for ArrayBuffer kinds. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/3512de18097c Move most of JS_ExternalizeArrayBufferContents into a static member function on ArrayBufferObject so that internals-observing code isn't smeared across two functions. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/432b2e88c651 Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/5be8cb19ad3d Remove ArrayBufferObject::hasData and perform its operation in its sole caller in a more straightforward manner. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/1477b8eb5442 Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/c9f62f10eeb5 Remove trailing arguments to ArrayBufferObject::createForContents that are identical for every caller. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e668a95d32 Make AllocateArrayBufferContents return uint8_t*, and make its callers consistently not redundantly report OOMs. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/e7fad41d68e0 Only pass BufferContents containing a non-null pointer to |ArrayBufferObject::createForContents|. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/deaa41ca96da Simplify ArrayBufferObject::createForContents some now that it's only ever passed non-null contents. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/d80ce47bba52 Consistently use "ArrayBuffer" instead of the vaguer "array buffer" in ArrayBuffer-related JSAPI descriptions. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc3cd982953 Add BufferContents::createFoo functions for every kind so that the templaty, harder-to-search-for create function can be removed. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/6f66e6c059df Remove ArrayBufferOffset::offsetOfFlags as unused. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/086849ef0dd5 Add BufferKind::NO_DATA for ArrayBuffers that have no data (whether because byteLength is zero or because the ArrayBuffer is detached), for which the value of |ownsData()| is irrelevant. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/d80b681a68e6 Initialize all ArrayBuffers that store their data inline using a single function. r=sfink
|
||
Comment 57•5 years ago
|
||
Backed out for spidermonkey bustages on /testAtomicOperations.cpp
backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/eed1098d0d6c9e3af5b02154295e452c6c21bb48
failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=229805289&repo=mozilla-inbound&lineNumber=2861
[task 2019-02-22T03:05:14.469Z] In file included from /builds/worker/workspace/build/src/obj-spider/js/src/jsapi-tests/Unified_cpp_js_src_jsapi-tests1.cpp:2:0:
[task 2019-02-22T03:05:14.469Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp: In member function 'virtual bool cls_testAtomicOperationsU8Clamped::run(JS::HandleObject)':
[task 2019-02-22T03:05:14.469Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:294:9: error: 'uint8_clamped' does not name a type
[task 2019-02-22T03:05:14.469Z] const uint8_clamped A(0xab);
[task 2019-02-22T03:05:14.469Z] ^~~~~~~~~~~~~
[task 2019-02-22T03:05:14.469Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:295:9: error: 'uint8_clamped' does not name a type
[task 2019-02-22T03:05:14.470Z] const uint8_clamped B(0x37);
[task 2019-02-22T03:05:14.470Z] ^~~~~~~~~~~~~
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:24: error: 'uint8_clamped' was not declared in this scope
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:283:3: note: in definition of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] T* q = (T*)hidePointerValue((void*)atomicMem);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:283:6: error: 'q' was not declared in this scope
[task 2019-02-22T03:05:14.470Z] T* q = (T*)hidePointerValue((void*)atomicMem);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:3: note: in expansion of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^~~~~~~~~~~~~~~~~~~~
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:283:13: error: expected primary-expression before ')' token
[task 2019-02-22T03:05:14.470Z] T* q = (T*)hidePointerValue((void*)atomicMem);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:3: note: in expansion of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^~~~~~~~~~~~~~~~~~~~
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:39: error: 'A' was not declared in this scope
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:284:8: note: in definition of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] q = A;
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:285:15: error: template argument 1 is invalid
[task 2019-02-22T03:05:14.470Z] SharedMem<T> p =
[task 2019-02-22T03:05:14.470Z] ^
[task 2019-02-22T03:05:14.470Z] /builds/worker/workspace/build/src/js/src/jsapi-tests/testAtomicOperations.cpp:296:3: note: in expansion of macro 'ATOMIC_CLAMPED_TESTS'
[task 2019-02-22T03:05:14.470Z] ATOMIC_CLAMPED_TESTS(uint8_clamped, A, B);
[task 2019-02-22T03:05:14.470Z] ^~~~~~~~~~~~~~~~~~~~
|
|
||
Comment 58•5 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/efd2d5e7ec57 Rename JS_NewArrayBufferWithExternalContents to JS_NewArrayBufferWithUserOwnedContents to better accord with the USER_OWNED ArrayBufferKind name, and to avoid namespace overload with the semantically distinct JS_NewExternalArrayBuffer and JS_ExternalizeArrayBufferContents functions. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/c0aee08543ad Split PLAIN into INLINE_DATA/MALLOCED for ArrayBuffer kinds. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5e10661a78 Move most of JS_ExternalizeArrayBufferContents into a static member function on ArrayBufferObject so that internals-observing code isn't smeared across two functions. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/76600605e1e3 Remove JS_ExternalizeArrayBufferContents because it has no users except in tests, implements complicated ownership semantics, and is definite implementation complexity. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/5a304d4b53c0 Remove ArrayBufferObject::hasData and perform its operation in its sole caller in a more straightforward manner. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/765bc7a86f72 Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/92fc3d9b6699 Remove trailing arguments to ArrayBufferObject::createForContents that are identical for every caller. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/51d373ab477b Make AllocateArrayBufferContents return uint8_t*, and make its callers consistently not redundantly report OOMs. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/4771a589408d Only pass BufferContents containing a non-null pointer to |ArrayBufferObject::createForContents|. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c09226e55b Simplify ArrayBufferObject::createForContents some now that it's only ever passed non-null contents. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/76e407233988 Consistently use "ArrayBuffer" instead of the vaguer "array buffer" in ArrayBuffer-related JSAPI descriptions. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdddf46e820 Add BufferContents::createFoo functions for every kind so that the templaty, harder-to-search-for create function can be removed. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/1cead3a873e2 Remove ArrayBufferOffset::offsetOfFlags as unused. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/3556d94cbc81 Add BufferKind::NO_DATA for ArrayBuffers that have no data (whether because byteLength is zero or because the ArrayBuffer is detached), for which the value of |ownsData()| is irrelevant. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/5b7a07b449ba Initialize all ArrayBuffers that store their data inline using a single function. r=sfink
|
|
Assignee | |
Comment 59•5 years ago
|
||
Comment on attachment 9045284 [details] [diff] [review] Rename the two ArrayBufferObject::create overloads to ArrayBufferObject::create{Zeroed,WithContents}, and inline a simplified form of the more-complex ArrayBufferObject::create into the new createZeroed function Review of attachment 9045284 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +1322,5 @@ > + } > + return nullptr; > + } > + > + MOZ_ASSERT(!gc::IsInsideNursery(buffer)); We ironed this out over IRC -- ArrayBuffers are never in the nursery because they have a finalizer, and not a nursery-finalizer-skippable one. I added a reason-string to this fresh call and to the one in ABO::createForContents, above, from which this was copied.
|
||
Comment 60•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/efd2d5e7ec57
https://hg.mozilla.org/mozilla-central/rev/c0aee08543ad
https://hg.mozilla.org/mozilla-central/rev/9e5e10661a78
https://hg.mozilla.org/mozilla-central/rev/76600605e1e3
https://hg.mozilla.org/mozilla-central/rev/5a304d4b53c0
https://hg.mozilla.org/mozilla-central/rev/765bc7a86f72
https://hg.mozilla.org/mozilla-central/rev/92fc3d9b6699
https://hg.mozilla.org/mozilla-central/rev/51d373ab477b
https://hg.mozilla.org/mozilla-central/rev/4771a589408d
https://hg.mozilla.org/mozilla-central/rev/b2c09226e55b
https://hg.mozilla.org/mozilla-central/rev/76e407233988
https://hg.mozilla.org/mozilla-central/rev/dcdddf46e820
https://hg.mozilla.org/mozilla-central/rev/1cead3a873e2
https://hg.mozilla.org/mozilla-central/rev/3556d94cbc81
https://hg.mozilla.org/mozilla-central/rev/5b7a07b449ba
|
|
Assignee | |
Updated•5 years ago
|
|
|
||
Comment 61•5 years ago
|
||
Comment on attachment 9045298 [details] [diff] [review] Remove |ArrayBufferObject::hasStealableContents()| and replace it with its contents, appropriately simplified for each calling location Review of attachment 9045298 [details] [diff] [review]: ----------------------------------------------------------------- This change seems safe, and it is great to see that function go. ::: js/src/vm/ArrayBufferObject.h @@ -351,5 @@ > static BufferContents stealContents(JSContext* cx, > Handle<ArrayBufferObject*> buffer, > bool hasStealableContents); > > - bool hasStealableContents() const { This function is the main reason I've been averse to touching anything ArrayBuffer related. I don't like having to re-think through the logic every time (especially when I'm still fuzzy on the wasm and asm.js constraints.)
|
|
||
Comment 62•5 years ago
|
||
Comment on attachment 9045302 [details] [diff] [review] Remove most uses of DoesntOwnData for NO_DATA and USER_OWNED and make current users that depend on |ownsData()| not do so Review of attachment 9045302 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +1711,5 @@ > + // pointer, so all that changes is the pointer's plain identity, and it seems > + // okay to say "don't do that". Ownership state is unchanged. And the > + // kind-change shouldn't matter because we only test > + // |buffer->hasUserOwnedData()| in ABO::prepareForAsmJS -- but only *after* > + // anything length-zero would have been filtered out, so no change there. Phew. I retraced through this logic, and it seems correct to me.
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Comment 63•5 years ago
|
||
Comment on attachment 9045313 [details] [diff] [review] Implement an ArrayBufferObject::extractStructuredCloneContents for structured-cloning an ArrayBuffer Review of attachment 9045313 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +1492,5 @@ > + MOZ_ASSERT_UNREACHABLE("bad kind when stealing malloc'd data"); > + break; > + } > + > + return BufferContents::createFailed(); Shouldn't there be another MOZ_ASSERT_UNREACHABLE for a bogus kind here? ::: js/src/vm/StructuredClone.cpp @@ +1875,2 @@ > if (!bufContents) { > return false; // out of memory Hm... there seems to be an OOM reporting problem here, in some earlier patch that I missed. This condition is now OOM or WASM, though the wasm case will never happen because https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1838 I see https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1838 that reports OOM, but then all the other paths do not. Given https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1903 it really seems like everything should report OOM. Perhaps this should be if (!bufContents) { MOZ_ASSERT(!arrayBuffer->isWasm()); ReportOutOfMemory(cx); return false; } at least for now? I'm still marking this r+ because the basic change is (very) good.
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Comment 64•5 years ago
|
||
Comment on attachment 9045320 [details] [diff] [review] Remove the useless OwnsState argument from ArrayBufferObject::createForContent Review of attachment 9045320 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +1258,5 @@ > // the ArrayBuffer for use as raw storage to store such information. > size_t reservedSlots = JSCLASS_RESERVED_SLOTS(&class_); > > size_t nslots = reservedSlots; > + if (true) { Heh, thanks.
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Comment 65•5 years ago
|
||
Comment on attachment 9045328 [details] [diff] [review] Remove ArrayBufferObject::change{,View}Contents as unused Review of attachment 9045328 [details] [diff] [review]: ----------------------------------------------------------------- Whoa, I somehow didn't expect that. Most excellent.
|
|
||
Comment 66•5 years ago
|
||
Comment on attachment 9045329 [details] [diff] [review] Remove the |newContents| argument from ArrayBufferObject::detach that's now identical for every caller Review of attachment 9045329 [details] [diff] [review]: ----------------------------------------------------------------- \o/ the last vestiges gone
|
|
||
Comment 67•5 years ago
|
||
Comment on attachment 9045330 [details] [diff] [review] Inline ArrayBufferObject::setNewData into its sole caller Review of attachment 9045330 [details] [diff] [review]: ----------------------------------------------------------------- That was an awesome refactoring series. Thank you for breaking it down to that level, it really helped.
|
|
||
Updated•5 years ago
|
|
|
||
Comment 68•5 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3ad3942d67 Remove |ArrayBufferObject::hasStealableContents()| and replace it with its contents, appropriately simplified for each calling location. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a4ce77f2f1 Remove most uses of DoesntOwnData for NO_DATA and USER_OWNED and make current users that depend on |ownsData()| not do so. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/f487c864d2ab Make NoteViewBufferWasDetached a lambda instead of a global function. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/715e9b139ebb Implement an ArrayBufferObject::stealMallocedContents for use in JS_StealArrayBufferContents, rather than reusing the hoary ArrayBufferObject::stealContents with finicky caller-side should-this-steal logic. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/a9638eeea757 Make ArrayBufferObject::wasmGrowToSizeInPlace do its work *not* using ArrayBufferObject::stealContents, rather using the exact operations desired, far more simply. r=sfink
|
|
Assignee | |
Comment 69•5 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #63)
::: js/src/vm/StructuredClone.cpp
@@ +1875,2 @@if (!bufContents) { return false; // out of memoryHm... there seems to be an OOM reporting problem here, in some earlier patch
that I missed.This condition is now OOM or WASM, though the wasm case will never happen
because
https://searchfox.org/mozilla-central/rev/
dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1838I see
https://searchfox.org/mozilla-central/rev/
dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1838
that reports OOM, but then all the other paths do not. Given
https://searchfox.org/mozilla-central/rev/
dbddac86aadf1d4871fb350bbe66db43728a9f81/js/src/vm/StructuredClone.cpp#1903
it really seems like everything should report OOM.Perhaps this should be
if (!bufContents) { MOZ_ASSERT(!arrayBuffer->isWasm()); ReportOutOfMemory(cx); return false; }at least for now?
As of this patch and the prior ones, you have
/* static */ ArrayBufferObject::BufferContents
ArrayBufferObject::extractStructuredCloneContents(
JSContext* cx, Handle<ArrayBufferObject*> buffer) {
CheckStealPreconditions(buffer, cx);
BufferContents contents = buffer->contents();
switch (contents.kind()) {
case INLINE_DATA:
case NO_DATA:
case USER_OWNED: {
uint8_t* copiedData = NewCopiedBufferContents(cx, buffer);
if (!copiedData) {
return BufferContents::createFailed();
}
ArrayBufferObject::detach(cx, buffer, BufferContents::createNoData());
return BufferContents::createMalloced(copiedData);
}
case MALLOCED:
case MAPPED: {
MOZ_ASSERT(contents);
// Overwrite the old data pointer *without* releasing old data.
BufferContents newContents = BufferContents::createNoData();
buffer->setDataPointer(newContents, OwnsData);
// Detach |buffer| now that doing so won't release |oldContents|.
ArrayBufferObject::detach(cx, buffer, newContents);
return contents;
}
case WASM:
JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
JSMSG_WASM_NO_TRANSFER);
return BufferContents::createFailed();
case EXTERNAL:
MOZ_ASSERT_UNREACHABLE(
"external ArrayBuffer shouldn't have passed the "
"structured-clone preflighting");
break;
case BAD1:
MOZ_ASSERT_UNREACHABLE("bad kind when stealing malloc'd data");
break;
}
MOZ_ASSERT_UNREACHABLE("garbage kind computed");
return BufferContents::createFailed();
}
The external/bad1/fallthrough cases can be silent halt to script execution, that's fine and perhaps even good. WASM will obviously set a pending exception. MALLOCED/MAPPED always return non-null contents. INLINE/NO/USER if NewCopiedBufferContents fails, it reported OOM -- I changed this earlier in this bug -- so we're good there. And then the copiedData returned is non-null.
So I don't think there's an OOM reporting problem here. I think you're comparing against current code without remembering everything all these other patches here have done?
|
|
||
Comment 70•5 years ago
|
||
Pushed by jwalden@mit.edu: https://hg.mozilla.org/integration/mozilla-inbound/rev/63536a044a29 Implement an ArrayBufferObject::extractStructuredCloneContents for structured-cloning an ArrayBuffer. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb592de5e03 Remove ArrayBufferObject::stealContents now that it's unused. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a39ff06158 Remove the useless OwnsState argument from ArrayBufferObject::changeContents. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/713088adfe5f Remove the useless OwnsState argument from ArrayBufferObject::setNewData. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/870a55710969 Remove the useless OwnsState argument from ArrayBufferObject::createForContent. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/f4101f442782 Inline the contents of a fresh |if (true) { ... }| in ArrayBufferObject::createForContent. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/a06864bc8352 Remove the useless OwnsState argument from ArrayBufferObject::initialize. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4c27821811 Remove the useless OwnsState argument from ArrayBufferObject::setDataPointer. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/650bd5a18809 Now that |ownsData()| is always true, inline that true value into all callers. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/694fe0c43793 Remove setOwnsData and the OWNS_DATA flag. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/973c3800d5d6 Remove ArrayBufferObject::change{,View}Contents as unused. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/ab967077f8a3 Remove the |newContents| argument from ArrayBufferObject::detach that's now identical for every caller. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/9103748036d1 Inline ArrayBufferObject::setNewData into its sole caller. r=sfink https://hg.mozilla.org/integration/mozilla-inbound/rev/bb879a6a95f8 Implement ArrayBufferObject::prepareForAsmJS using a switch, not a series of ifs that's less obviously exhaustive. r=sfink
|
|
Assignee | |
Comment 71•5 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #67)
That was an awesome refactoring series. Thank you for breaking it down to
that level, it really helped.
Didn't much have a choice, as I wouldn't have been smart enough to do it any other way. :-) Gordian knot, you have to pull a little bit at a time to get the whole thing untangled given the unholy kind/ownership pairing.
|
||
Comment 72•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/ac3ad3942d67
https://hg.mozilla.org/mozilla-central/rev/d8a4ce77f2f1
https://hg.mozilla.org/mozilla-central/rev/f487c864d2ab
https://hg.mozilla.org/mozilla-central/rev/715e9b139ebb
https://hg.mozilla.org/mozilla-central/rev/a9638eeea757
https://hg.mozilla.org/mozilla-central/rev/63536a044a29
https://hg.mozilla.org/mozilla-central/rev/5cb592de5e03
https://hg.mozilla.org/mozilla-central/rev/a7a39ff06158
https://hg.mozilla.org/mozilla-central/rev/713088adfe5f
https://hg.mozilla.org/mozilla-central/rev/870a55710969
https://hg.mozilla.org/mozilla-central/rev/f4101f442782
https://hg.mozilla.org/mozilla-central/rev/a06864bc8352
https://hg.mozilla.org/mozilla-central/rev/ef4c27821811
https://hg.mozilla.org/mozilla-central/rev/650bd5a18809
https://hg.mozilla.org/mozilla-central/rev/694fe0c43793
https://hg.mozilla.org/mozilla-central/rev/973c3800d5d6
https://hg.mozilla.org/mozilla-central/rev/ab967077f8a3
https://hg.mozilla.org/mozilla-central/rev/9103748036d1
https://hg.mozilla.org/mozilla-central/rev/bb879a6a95f8
Description
•