buffer: remove MAX_SAFE_INTEGER check on length

[Trott]

MAX_SAFE_INTEGER is millions of times larger than the largest buffer
allowed in Node.js. There is no need to squash the length down to
MAX_SAFE_INTEGER. Removing that check results in a small but
statistically significant increase for Buffer.from() operating on
ArrayBuffers in some situations.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

buffer

[Trott]

                                                                    improvement confidence     p.value
 buffers/buffer-from.js n=2048 len=10 source="arraybuffer-middle"        1.54 %         ** 0.001666215
 buffers/buffer-from.js n=2048 len=2048 source="arraybuffer-middle"      1.76 %         ** 0.003719416

[Trott]

If you do somehow manage to get Number.MAX_SAFE_INTEGER in there, the new FastBuffer() at the end of the function results in:

RangeError: Invalid typed array length: 9007199254740991

Before, if you manage to go here, say with Number.MAX_VALUE, you'd still get the error above because it got squashed down to Number.MAX_SAFE_INTEGER. Now you get:

RangeError: Invalid typed array length: 1.7976931348623157e+308

This seems more correct to me. You're getting the actual length that is problematic rather than some other length that you didn't specify.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/9036/

[mscdex]

Is this technically semver-major due to the possible error change for large values?

[bnoordhuis]

Is this technically semver-major due to the possible error change for large values?

No, IMO.

[Trott]

The errors in buffer module is migrating to use internal/errors, so this PR may have conflicts with #13976 , which has a new code ERR_BUFFER_OUT_OF_BOUNDS for the RangerError here.

This PR doesn't change the RangeError code. It deletes two lines untouched by #13976.

[refack]

Is this technically semver-major due to the possible error change for large values?

Depends on resolution of #13937

[refack]

Removing that check results in a small but
statistically significant increase for Buffer.from() operating on
ArrayBuffers in some situations.

A ~1.5% of the time was used for that if... Very unintuitive. Makes me put the meaning of our micro-benchmarks into new proportions, they seem to be very node core specific with limited real world correlation.
Ref: nodejs/CTC#146

[Trott]

Is this technically semver-major due to the possible error change for large values?
No, IMO.

I agree. If the wording changed, then yes. But since it's just correcting the (IMO) wrong value repeated back to the user, I would say no.

Depends on resolution of #13937

I don't think so because that's about changes after we move to the new internal error system. This one is not part of the new internal error system.

[Trott]

Landed in e6e6b07

[MylesBorins]

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

[Trott]

@MylesBorins Did you mean to label this either backport-requested or dont-land-on instead of backported?

[Trott]

(Changed to backport requested..)

[Trott]

@MylesBorins If #12361 lands in v6.x-staging, then this should land and will land cleanly. If #12361 does not land in v6.x-staging, then neither should this. #12361 is currently marked for lts-watch. I'm going to remove the backport-requested and add an lts-watch label for this.