1698791 - Add an is-index flag to atoms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

JSLinearString::isIndex is very hot, it's used for megamorphic GetElem lookups for example (to convert an atom to a jsid).

Non-index strings are the common case there, but we need at least 6 branches to figure that out: hasIndexValue, length == 0, length > UINT32_CHAR_BUFFER_LENGTH, inline vs non-inline chars, Latin1 vs TwoByte chars, IsAsciiDigit.

If we instead determine isIndex when we allocate a new JSAtom, we can reduce this to a single flag check. Furthermore, it makes the index-value stored in the upper 16 flag bits more effective, because we will set it for all index-atoms <= UINT16_MAX.

I collected some data on the number of isIndexSlow (or equivalent) calls before/after this change. When browsing some popular websites, it goes from 2-3 million to 0.1-0.2 million, so it's a good trade-off.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1698791 part 1 - Replace JSLinearString::isIndexSlow with CheckStringIsIndex. r?evilpie!

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1698791 part 2 - Use isIndex/CheckStringIsIndex for StringIsArrayIndex. r?evilpie!

StringIsArrayIndex is almost identical to isIndex/CheckStringIsIndex, but it's
limited to UINT32_MAX - 2 instead of UINT32_MAX.

This patch de-duplicates this with an explicit check for the smaller max length.
This ensures we can benefit from the index-value optimization and the ATOM_IS_INDEX
flag added in the next patch.

The difference in maximum length is still confusing and should probably be cleaned
up separately.

Depends on D108741

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1698791 part 3 - Add an is-index flag to JS atoms. r?evilpie!

This lets us speed up JSAtom to PropertyKey conversions. This matters for example
for megamorphic GetElem lookups (a very hot code path for modern JS frameworks).

Although this calls isIndexSlow for each atom we create, it still results in a lot
less calls to that method compared to before, when browsing some popular websites.

Depends on D108742

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48b921d8aeee
part 1 - Replace JSLinearString::isIndexSlow with CheckStringIsIndex. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/467a7afcdd3d
part 2 - Use isIndex/CheckStringIsIndex for StringIsArrayIndex. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/1bd16d8959e5
part 3 - Add an is-index flag to JS atoms. r=evilpie

Comment 5

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=1bd16d8959e5450ed6fa84f56bca4c14a3befa27&searchStr=linux%2Cx64%2Copt%2Cvalgrind-linux64-valgrind%2Fopt%2Cv&selectedTaskRun=GD-eV06yQLmJZTFBdRxq2g.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=334017796&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/26f36067564cca38a530b6edc62a09481848de38

Comment 6

[Jan de Mooij [:jandem]]

This is likely a false positive. In GetIndexFromString I changed the first line here to uint32_t index;:

  uint32_t index = UINT32_MAX;
  if (!str->asLinear().isIndex(&index) || index > INT32_MAX) {
    return -1;
  }

But looking at the history of that code that was a workaround for Valgrind (bug 1481670) :/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1c00e47dfce
part 1 - Replace JSLinearString::isIndexSlow with CheckStringIsIndex. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/190e231ffdeb
part 2 - Use isIndex/CheckStringIsIndex for StringIsArrayIndex. r=evilpie
https://hg.mozilla.org/integration/autoland/rev/ab1644a7b0ee
part 3 - Add an is-index flag to JS atoms. r=evilpie

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d1c00e47dfce
https://hg.mozilla.org/mozilla-central/rev/190e231ffdeb
https://hg.mozilla.org/mozilla-central/rev/ab1644a7b0ee