Replace all u64/i64 for length, index or offsets into usize/isize #4083
Conversation
On platforms that are 32-bits (or 128?) this should make array and iterations faster in general. On platforms that are 64-bits, this will have no effect.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4083 +/- ##
==========================================
+ Coverage 47.24% 53.34% +6.09%
==========================================
Files 476 484 +8
Lines 46892 48196 +1304
==========================================
+ Hits 22154 25709 +3555
+ Misses 24738 22487 -2251 ☔ View full report in Codecov by Sentry. |
Test262 conformance changes
Broken tests (9): |
|
Hmm, there were some test regressions. Can you investigate them? Maybe it was just a typo somewhere. |
|
@jedel1043 I don't know how to manually re-run the test262 on this project, so I'll leave that to you as a reviewer. Thanks! |
|
@hansl tests already ran, it's just on the output of the "Update comment / Write a new comment" steps for the Test262 action. We really need to setup something so that this thing works on external repos 😅 |
Test262 conformance changes
|
|
Scratch that. I won't be able to easily run test262, but I'll run the integration tests and the unit tests tonight and report back. Will also create an issue. |
|
At least 3 tests are failing in 
To be fair, they're likely wrong test expectations based on overflow/underflow, but I still want to make sure. |
core/engine/src/value/mod.rs
Outdated
| pub fn to_length(&self, context: &mut Context) -> JsResult<usize> { | ||
| // 1. Let len be ? ToInteger(argument). | ||
| // 2. If len ≤ +0, return +0. | ||
| // 3. Return min(len, 2^53 - 1). |
There was a problem hiding this comment.
We used to have it be usize, but we changed it because on 32-bit systems it does not capture the full range that a JavaScript length can be. (for arrays it's fine to put it as usize since it's between 0..2^32-1, but for strings and typed arrays, the upper bound is 2^53-1 (max safe representable integer in a f64).
Setting it to usize truncates the values on 32-bit platforms giving wrong results.
There was a problem hiding this comment.
For example, one place that to_length is used is String.padEnd() on the maxLength parameter, putting 2^32-1 would work on 32-bit platform anything greater will be truncated (i.e. 2^32 will be truncated to 0), on 64-bit platform this would be fine.
There was a problem hiding this comment.
MAX_STRING_LENGTH is 2^32-1.
From the MDN:
The language specification requires strings to have a maximum length of 253 - 1 elements, which is the upper limit for precise integers. However, a string with this length needs 16384TiB of storage, which cannot fit in any reasonable device's memory, so implementations tend to lower the threshold, which allows the string's length to be conveniently stored in a 32-bit integer.
In V8 (used by Chrome and Node), the maximum length is 229 - 24 (~1GiB). On 32-bit systems, the maximum length is 228 - 16 (~512MiB).
In Firefox, the maximum length is 230 - 2 (~2GiB). Before Firefox 65, the maximum length was 228 - 1 (~512MiB).
In Safari, the maximum length is 231 - 1 (~4GiB).
There was a problem hiding this comment.
The issue isn't with allocation of the strings, rather that it breaks the math and checks in many places.
EDIT: What I mean is that even if the allocation is limited, the spec still assumes at the length is the full range, and all the math and checks before the final allocation should be able to hold that range.
There was a problem hiding this comment.
That's all fine. If there's a bug with math being broken, then we need to fix it.
I would assume test262 and/or our tests would find any discrepancies, and I'd consider any issue a blocker for this PR. I'm currently running the full test262 suite on a Rasp Pi 2 equivalent. Everything works locally on my aarch64 Mac.
There was a problem hiding this comment.
If you can provide me with a valid JavaScript snippet that works on other implementations but doesn't work on this PR, I'll be more than happy to add it to tests and fix this PR.
|
Okay so after running test262 a bunch, and testing in browsers and Deno, I would still like to build this PR the right way but will need to move to a different approach:
This should still result in performance improvements on 32-bits platforms (e.g. in wasm32). I may type the new sizes For now this PR will be back to draft. |
On platforms that are 32-bits (or 128?) this should make array and iterations faster in general. On platforms that are 64-bits, this will have no effect.