1908073 - Make uint8_clamped and float16 more like built-in scalar types

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[André Bargull [:anba]]

Adding std::numeric_limits specialisations for uint8_clamped and float16 and adding some cast operators allow to remove some code where we have to explicitly handle uint8_clamped or float16. Furthermore perform some clean-ups in ElementSpecific to reduce duplicated code and avoid generating unreachable code paths.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 1: Remove unnecessary special-case for NaN. r=#spidermonkey-reviewers!

uint8_clamped, ToUint32, and ToInt32 all handle NaN values by returning zero,
so there's no need to special case NaN values.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 2: Remove unnecessary explicit definition of DataViewObject::read for float16. r=#spidermonkey-reviewers!

js::float16 doesn't allow implicit construction from int32_t, so it can't use
NativeType val = 0. Replacing the = 0 initialization with NativeType val{}
makes the code compatible with js::float16 and avoids the explicit definition
for DataViewObject::read.

Depends on D216653

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 3: Make uint8_clamped and float16 more like built-in scalar types. r=#spidermonkey-reviewers!

• Allow casting float16 to float and double, which allows sharing more code
  with built-in floating point types.
• Make {uint8_clamped,float16}::val private, because it's an implementation detail.
• Add float16::{from,to}RawBits() when the bit-pattern has to be accessed. (No longer
  needed when we can use C++20 constexpr bit-casting.)
• Make uint8_clamped constructors constexpr. constexpr constructors need to
  initialise all members in C++17, so we can no longer call the assignment operators
  in the constructor. The default constructor can't be constexpr, because then the
  class is no longer trivial, which breaks memcpy/memset optimisations.
• Make similar changes to float16 for consistency.

Depends on D216654

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 4: Replace TypeIs{FloatingPoint,Unsigned} with std::numeric_limits. r=#spidermonkey-reviewers!

Instead of just commenting that std::numeric_limits can't be used, specialise
std::numeric_limits for both uint8_clamped and float16.

Specialising std definitions is in most cases UB, but there are a few exceptions
like std::numeric_limits, which is explicitly allowed to be specialised by
user-code.

Note:

• Upstream also provides std::numeric_limits<float16>, but uses different constants
  for quiet and signaling NaN. Instead use quiet and signaling NaNs which match x86
  and ARM.

Drive-by change:

• Consistently use std::is_same_v instead of TypeIDOfType in ElementSpecific.

Depends on D216655

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 5: Make ConvertNumber more readable. r=#spidermonkey-reviewers!

There are too many ConvertNumber functions. Use if constexpr so this code
is easier to understand.

And also use ConvertNumber in two additional places.

Depends on D216656

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 6: Avoid duplicated store code in ElementSpecific. r=#spidermonkey-reviewers!

Avoid repeating the same store loops multiple times.

Drive-by changes:

• Consistently use Ops::extract within ElementSpecific.
• Replace manual malloc/free with a unique-ptr.

Depends on D216657

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 7: Use bitwise copying in SetFromTypedArray when the bit-level representation is compatible. r=#spidermonkey-reviewers!

TypedArraySlice already used bitwise copying when the bit-level representation is
compatible. SetFromTypedArray can reuse this approach.

Depends on D216658

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 8: Avoid generating unused code in ElementSpecific::store. r=#spidermonkey-reviewers!

SetFromTypedArray was already too large before BigInt TypedArrays were added (bug 1093292).

Add template code to avoid generating unreachable code. Add if constexpr in a few places
and then remove conversions in ConvertNumber which aren't even allowed in JS.

Depends on D216659

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 9: Share UnsignedSortValue for all floating point types. r=#spidermonkey-reviewers!

mozilla::FloatingPoint is marked final, so we have to use composition instead
of inheritance to provide a definition for float16.

Depends on D216660

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1908073 - Part 10: Use std::numeric_limits for TypedArraySort type checks. r=#spidermonkey-reviewers!

Use the std::numeric_limits specialisations from part 4 for this code, too.

Depends on D216661

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0ff83155a3e
Part 1: Remove unnecessary special-case for NaN. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/ffe61ffdcd68
Part 2: Remove unnecessary explicit definition of DataViewObject::read for float16. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/1c286a94d944
Part 3: Make uint8_clamped and float16 more like built-in scalar types. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/549633d0cedb
Part 4: Replace TypeIs{FloatingPoint,Unsigned} with std::numeric_limits. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/bd3492b52000
Part 5: Make ConvertNumber more readable. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/328b48e7b7f3
Part 6: Avoid duplicated store code in ElementSpecific. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/be2ea91796de
Part 7: Use bitwise copying in SetFromTypedArray when the bit-level representation is compatible. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/f4c250fa5f5f
Part 8: Avoid generating unused code in ElementSpecific::store. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/26252b089885
Part 9: Share UnsignedSortValue for all floating point types. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/2c90b013e9f1
Part 10: Use std::numeric_limits for TypedArraySort type checks. r=spidermonkey-reviewers,jandem

Comment 12

[Cristina Horotan [:chorotan]]

Backed out 12 changesets (bug 1908073, bug 1908074) for causing build bustages at TypedArrayObject-inl.h

Backout: https://hg.mozilla.org/integration/autoland/rev/6b79ec6e910bdfb82d2cb864af7cf92b5d984ab6

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=e89589a41c198c4966a35531e2396cfbd699cdfe&selectedTaskRun=ZDzVM65sShC1AU6EbsoNpw.0

Failure push: https://treeherder.mozilla.org/logviewer?job_id=467489363&repo=autoland&lineNumber=40959

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/654a038c6c48
Part 1: Remove unnecessary special-case for NaN. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/435659f830fd
Part 2: Remove unnecessary explicit definition of DataViewObject::read for float16. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/196bea2d1aee
Part 3: Make uint8_clamped and float16 more like built-in scalar types. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/38d6c8dac6d6
Part 4: Replace TypeIs{FloatingPoint,Unsigned} with std::numeric_limits. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/196ebdccf7ce
Part 5: Make ConvertNumber more readable. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/a00d966c511f
Part 6: Avoid duplicated store code in ElementSpecific. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/88a9ab51d7b3
Part 7: Use bitwise copying in SetFromTypedArray when the bit-level representation is compatible. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/a1468772cc02
Part 8: Avoid generating unused code in ElementSpecific::store. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/5441251b3d31
Part 9: Share UnsignedSortValue for all floating point types. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/57777156ea4a
Part 10: Use std::numeric_limits for TypedArraySort type checks. r=spidermonkey-reviewers,jandem

Comment 14

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/654a038c6c48
https://hg.mozilla.org/mozilla-central/rev/435659f830fd
https://hg.mozilla.org/mozilla-central/rev/196bea2d1aee
https://hg.mozilla.org/mozilla-central/rev/38d6c8dac6d6
https://hg.mozilla.org/mozilla-central/rev/196ebdccf7ce
https://hg.mozilla.org/mozilla-central/rev/a00d966c511f
https://hg.mozilla.org/mozilla-central/rev/88a9ab51d7b3
https://hg.mozilla.org/mozilla-central/rev/a1468772cc02
https://hg.mozilla.org/mozilla-central/rev/5441251b3d31
https://hg.mozilla.org/mozilla-central/rev/57777156ea4a

Comment 15

[Andrej (:aglavic)]

(In reply to Pulsebot from comment #13)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/654a038c6c48
Part 1: Remove unnecessary special-case for NaN.
r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/435659f830fd
Part 2: Remove unnecessary explicit definition of DataViewObject::read for
float16. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/196bea2d1aee
Part 3: Make uint8_clamped and float16 more like built-in scalar types.
r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/38d6c8dac6d6
Part 4: Replace TypeIs{FloatingPoint,Unsigned} with std::numeric_limits.
r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/196ebdccf7ce
Part 5: Make ConvertNumber more readable. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/a00d966c511f
Part 6: Avoid duplicated store code in ElementSpecific.
r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/88a9ab51d7b3
Part 7: Use bitwise copying in SetFromTypedArray when the bit-level
representation is compatible. r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/a1468772cc02
Part 8: Avoid generating unused code in ElementSpecific::store.
r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/5441251b3d31
Part 9: Share UnsignedSortValue for all floating point types.
r=spidermonkey-reviewers,jandem
https://hg.mozilla.org/integration/autoland/rev/57777156ea4a
Part 10: Use std::numeric_limits for TypedArraySort type checks.
r=spidermonkey-reviewers,jandem

Perfherder has detected a talos performance change from push e28012505de56dc0f292888bf2ce9b4d8102d99d.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
14%	pdfpaint xfa_bug1722030_1.pdf	macosx1015-64-shippable-qr	e10s fission stylo webrender	9,146.19 -> 7,910.52
13%	pdfpaint xfa_bug1722030_1.pdf	linux1804-64-shippable-qr	e10s fission stylo webrender-sw	8,236.90 -> 7,139.43
13%	pdfpaint xfa_bug1722030_1.pdf	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	9,293.08 -> 8,056.03
12%	pdfpaint xfa_bug1722030_1.pdf	linux1804-64-shippable-qr	e10s fission stylo webrender	8,238.58 -> 7,255.53
10%	perf_reftest_singletons external-string-pass.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	794.46 -> 718.65
...	...	...	...	...
2%	pdfpaint issue16263.pdf	windows11-64-shippable-qr	e10s fission stylo webrender-sw	4,532.44 -> 4,451.23

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 1594

For more information on performance sheriffing please see our FAQ.