Equalize TypedArray and ArrayBuffer max lengths [v8/v8 : main]
Shu-yu Guo (Gerrit)
Attention is currently required from: Igor Sheludko, Marja Hölttä.
1 comment:
Patchset:
marja@ found an issue with resizable buffers where the different max lengths cause problems. If possible we should make them the same, as the TODO says.
ishell@, v8:4153 looks mostly done, but we never actually made the TypedArray and ArrayBuffer max lengths equal. All the V8 tests pass (ignoring the downstream failures in blink and Node). Am I missing anything or is it actually this easy? 😊
If I'm not missing anything, I'll fix the downstream issues.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Marja Hölttä (Gerrit)
Attention is currently required from: Igor Sheludko, Shu-yu Guo.
1 comment:
Patchset:
marja@ found an issue with resizable buffers where the different max lengths cause problems. […]
Re: "is it actually this easy", if there were problems with this approach, I don't think you'd find them out, in the current form of the CL.
There are no existing tests that use large TypedArrays in the code base, since they aren't currently allowed. And this CL doesn't add any tests. So just changing the constant is probably a NOOP.
I'd guess this is most likely to break when somebody creates a large TypedArray and then it flows to some part of the code which assumes the length is a SMI. Possibly that'd happen in optimized code.
What would be the acceptable minimum testing before we can conclude this probably works? At least testing element access + optimizing it for large TypedArrays. Maybe also calling all possible TypedArray.prototype (and Array.prototype?) functions with large TypedArrays, and making sure those also get optimized?
If you look for v8:4153, there are some comments saying "support large TypedArrays here" which sounds like they're not supported at the moment. They are in various TA.p functions which makes it sound like we'd need testing for all of them to be convinced this works...
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Igor Sheludko (Gerrit)
Attention is currently required from: Marja Hölttä, Shu-yu Guo.
1 comment:
Patchset:
Re: "is it actually this easy", if there were problems with this approach, I don't think you'd find […]
+1 to Marja's points, it's worth going over TODOs first. For example, TypedArrayPrototypeSort's implementation is not suitable for huge typed arrays at all.
Re testing, we have a --mock-arraybuffer-allocator flag for testing some basic functionality for huge typed arrays but it helps only in very simple scenarios.
IIRC we discussed this issue with Jakob before, and the idea was to consider throwing an error when TA.p methods see unreasonably huge typed array.CC'ing Jakob, who had been working on supporting huge typed arrays in the past.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Igor Sheludko, Marja Hölttä.
1 comment:
Patchset:
+1 to Marja's points, it's worth going over TODOs first. […]
Thanks for the tips, I'll look into the testing and go through the TODOs. Testing seems pretty gnarly, since we don't want to actually be allocating sorting 9-petabyte TAs.
I don't quite understand the Smi point, however, since the hard work was already done to make max TA length 2^32 on 64bit archs, which already exceeds the Smi range. I looked a bit at TF's assumptions as well but the the TA length has a custom range type already AFAICT.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Igor Sheludko (Gerrit)
Attention is currently required from: Marja Hölttä, Shu-yu Guo.
1 comment:
Patchset:
Thanks for the tips, I'll look into the testing and go through the TODOs. […]
I think the main reasons why we didn't increase the limit back then was inability to test this properly + a lack of demand/motivation from users.
IIRC the main use case we were caring about was supporting 4GB ArrayBuffers for Wasm.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Jakob Kummerow (Gerrit)
Attention is currently required from: Igor Sheludko, Marja Hölttä, Shu-yu Guo.
1 comment:
Patchset:
I think the main reasons why we didn't increase the limit back then was inability to test this prope […]
A couple of thoughts:
- On 32-bit platforms, 1 GiB is plenty. It is practically impossible to find 2 GiB of contiguous free _address space_ (even aside from free physical memory) in real-world, non-toy processes. Even finding 256MiB often fails when it's been a while since renderer startup and address space has experienced fragmentation.
- 64-bit platforms, despite their name, use 48 bits for virtual addresses (of which the kernel typically reserves one for itself); the number of bits for physical addresses depends on specific hardware and in current CPUs typically ranges from 35 (lightweight laptops) to 42 (servers). Additionally, Chrome has a 100 TiB per renderer sandbox limit. So going to 2**53 is absolute overkill, and will be for decades to come (sure, memory grows every year, but look at the past two decades and make an extrapolating guess how that flattening curve will continue!). I'd keep `kMaxSafeInteger` in `v8::internal`.
- One reason against raising V8's limits too much is that, given sufficient virtual address space, allocations will typically appear to succeed, and only writing to those array elements will cause the kernel to actually reserve the pages and potentially crash the process when that fails. Since "RangeError" is a better UX/DX than "Aw, snap!", it's not necessarily a good idea to allow, say, 128 GiB for everyone just because a few expensive workstations might actually have that much memory.
- Keep in mind that a Float64Array with length `N` needs an ArrayBuffer with length `N*8` as its backing store. Perfect equality of limits is hence elusive.
- There should already be no places in the code that attempt to store TypedArray lengths in Smis without checking first. That said, AFAIK there are some builtins that will simply refuse to work on overly large arrays and throw an exception instead. This can already be triggered, so shouldn't block raising the limit further, but we should address these eventually. (There appears to be very little demand.)
- For testing, we have `--mock-arraybuffer-allocator` (see d8.h/cc), which repeatedly maps the same small memory chunk into a huge address range. It has limitations (Linux only, don't call `std::sort` on it, expect array elements to alias each other), and you won't be able to allocate 9 PiB due to virtual address limits (see above).
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Igor Sheludko, Jakob Kummerow, Marja Hölttä.
1 comment:
Patchset:
A couple of thoughts: […]
Thanks for the detailed thoughts Jakob!
I should've led with the motivation, which is not at all about any particular limit, but just that the TA max length and the AB max length are exactly the same. For resizable buffers, marja@ spotted an issue if the max lengths differ. With resizable buffers, you can have "length-tracking" TAs that track the underlying AB's byte length. So if you have different max lengths, you'd need to throw somewhere for ABs whose max lengths are > TA max lengths. Otherwise, if you have e.g. a length-tracking `Int8Array`, resizes can make it exceed the max TA length. That seems kind of gross if we can make the max lengths the same, whatever that is.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Igor Sheludko, Jakob Kummerow, Marja Hölttä.
1 comment:
Patchset:
Thanks for the detailed thoughts Jakob! […]
Oops, butchered the last sentence: that seems kind of gross. If we can make the max lengths the same, whatever that is, then we can avoid having to throw somewhere.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Igor Sheludko, Jakob Kummerow, Marja Hölttä.
1 comment:
Patchset:
Oops, butchered the last sentence: that seems kind of gross. […]
Given all of this, it sounds like we could also _lower_ the max AB length to be 2^32 instead? I wonder if there are uses of ABs > 4G.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Jakob Kummerow (Gerrit)
Attention is currently required from: Igor Sheludko, Marja Hölttä, Shu-yu Guo.
1 comment:
Patchset:
Given all of this, it sounds like we could also _lower_ the max AB length to be 2^32 instead? I wond […]
I don't think there can be any such uses yet, because there's no way to construct larger ABs.
- Wasm Memories are capped to 4GiB currently (though Clemens is looking into raising that to 8 or possibly 16 GiB for "wasm64", so capping ABs to 4 GiB is not an option going forward).
- PartitionAlloc won't give you more than ~2.1 GiB (if I recall the limit correctly) for direct `new ArrayBuffer(N)` construction.
- That said, I'm not sure about Node and other non-Chrome embedders.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Marja Hölttä (Gerrit)
Attention is currently required from: Igor Sheludko, Jakob Kummerow, Shu-yu Guo.
1 comment:
Patchset:
I don't think there can be any such uses yet, because there's no way to construct larger ABs. […]
Re: "I don't think there can be any such uses yet, because there's no way to construct larger ABs."
It's currently possible to construct a large buffer but use TAs to view only a portion of it at a time.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Samuel Groß (Gerrit)
Attention is currently required from: Igor Sheludko, Jakob Kummerow, Shu-yu Guo.
1 comment:
Patchset:
Maybe not super relevant right now, but the V8 sandbox currently effectively limits ArrayBuffer, TypedArray, etc. sizes to 32GB: https://docs.google.com/document/d/1PM4Zqmlt8ac5O8UNQfY7fOsem-6MhbsB-vjFI-9XK6w/edit#heading=h.e68bw3kmgamy so maybe we should use that as the limit (when V8_ENABE_SANDBOX is active).
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Clemens Backes (Gerrit)
Attention is currently required from: Igor Sheludko, Jakob Kummerow, Shu-yu Guo.
1 comment:
Patchset:
Jakob made me aware of this CL (I hit the TA limit when testing 16GB wasm memories). Wasm memory64 is still experimental, but long-term it would surely be unexpected if we can get the 16GB AB but not create a TA from it.
Equalizing all limits to the maximum you can allocate from V8 (JS or Wasm) sounds like a good path forward. That would be 16GB for now, and would maybe have to be raised by another small factor in a few years.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Shu-yu Guo abandoned this change.
To view, visit change 3684809. To unsubscribe, or for help writing mail filters, visit settings.