1746380 - Split string allocation code for atom and non-atom case

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Tooru Fujisawa [:arai]]

There are cx->zone()->isAtomsZone() branch deep inside string allocation code.
The condition becomes true only when the caller has AutoAllocInAtomsZone or JSContext::enterAtomsZone.
and now we have very limited set of callsite that allocates atoms.
we should be able to split the allocation code for each, maybe by templatizing the functions if necessary, to optimize both atom case and non-atom case.

https://searchfox.org/mozilla-central/rev/b80b92e2e10299ab405bdc4eb5109e9e23e25160/js/src/vm/StringType-inl.h#253-266

template <js::AllowGC allowGC, typename CharT>
MOZ_ALWAYS_INLINE JSLinearString* JSLinearString::new_(
    JSContext* cx, js::UniquePtr<CharT[], JS::FreePolicy> chars, size_t length,
    js::gc::InitialHeap heap) {
  if (!validateLength(cx, length)) {
    return nullptr;
  }

  JSLinearString* str;
  if (cx->zone()->isAtomsZone()) {
    str = js::Allocate<js::NormalAtom, allowGC>(cx);
  } else {
    str = js::AllocateString<JSLinearString, allowGC>(cx, heap);
  }

https://searchfox.org/mozilla-central/rev/b80b92e2e10299ab405bdc4eb5109e9e23e25160/js/src/vm/StringType-inl.h#308-315

template <js::AllowGC allowGC>
MOZ_ALWAYS_INLINE JSThinInlineString* JSThinInlineString::new_(
    JSContext* cx, js::gc::InitialHeap heap) {
  if (cx->zone()->isAtomsZone()) {
    return (JSThinInlineString*)(js::Allocate<js::NormalAtom, allowGC>(cx));
  }

  return js::AllocateString<JSThinInlineString, allowGC>(cx, heap);

https://searchfox.org/mozilla-central/rev/b80b92e2e10299ab405bdc4eb5109e9e23e25160/js/src/vm/StringType-inl.h#318-325

template <js::AllowGC allowGC>
MOZ_ALWAYS_INLINE JSFatInlineString* JSFatInlineString::new_(
    JSContext* cx, js::gc::InitialHeap heap) {
  if (cx->zone()->isAtomsZone()) {
    return (JSFatInlineString*)(js::Allocate<js::FatInlineAtom, allowGC>(cx));
  }

  return js::AllocateString<JSFatInlineString, allowGC>(cx, heap);

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Bug 1746380 - Part 1: Clarify atomization progress in the function name. r?tcampbell!

To avoid duplicate steps through the atomization process:

• If the function assumes the input doesn't match static strings
  (static strings case is already handled), append NoStatic
• If the function assumes the length is valid, append ValidLength

Depends on D135099

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1746380 - Part 2: Move atom-specific path out of string allocation code. r?tcampbell!

Split cx->zone()->isAtomsZone() branch into 2 functions, for atom case and
non-atom case.

Also removed reduncent steps (static string lookup, length validation,
deflation) from atom case, based on the atomization progress.

Depends on D135100

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Bug 1746380 - Part 3: Remove length validation from atom instantiation. r?tcampbell!

Depends on D135101

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Bug 1746380 - Part 4: Remove length validation and static string lookup from non-atom string instantiation. r?tcampbell!

Depends on D135102

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1746380 - Part 5: Optimize well-known symbol construction. r?tcampbell!

Depends on D135103

Comment 6

[Tooru Fujisawa [:arai]]

the stack of bug 1745664, bug 1746382, and this bug improves the atomization performance by 2% for both self-case (InstantiateMarkedAtomsAsPermanent) and normal case (InstantiateMarkedAtoms).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eeb21b8d0aa9
Part 1: Clarify atomization progress in the function name. r=nbp
https://hg.mozilla.org/integration/autoland/rev/006da995143d
Part 2: Move atom-specific path out of string allocation code. r=nbp
https://hg.mozilla.org/integration/autoland/rev/cafa79c30500
Part 3: Remove length validation from atom instantiation. r=nbp
https://hg.mozilla.org/integration/autoland/rev/9e4cdff09263
Part 4: Remove length validation and static string lookup from non-atom string instantiation. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7269c2e3b6f8
Part 5: Optimize well-known symbol construction and remove unnecessary check for null-zone. r=nbp

Comment 8

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing multiple failures. CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/5f0294c305465f40899be2e0fcc508de594c5b45

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=7269c2e3b6f8fffd62c5c30bb7a0b72739d68e81&selectedTaskRun=M-hOPyTtQ4qyRpr4g40jOw.0

Link to failure log:
https://treeherder.mozilla.org/logviewer?job_id=364446492&repo=autoland&lineNumber=97820
https://treeherder.mozilla.org/logviewer?job_id=364446456&repo=autoland&lineNumber=89217
https://treeherder.mozilla.org/logviewer?job_id=364446482&repo=autoland&lineNumber=81388
https://treeherder.mozilla.org/logviewer?job_id=364448493&repo=autoland&lineNumber=1513
https://treeherder.mozilla.org/logviewer?job_id=364448500&repo=autoland&lineNumber=804
https://treeherder.mozilla.org/logviewer?job_id=364448486&repo=autoland&lineNumber=1677
https://treeherder.mozilla.org/logviewer?job_id=364448542&repo=autoland&lineNumber=1711
https://treeherder.mozilla.org/logviewer?job_id=364448521&repo=autoland&lineNumber=1697

Comment 9

[Tooru Fujisawa [:arai]]

turns out that we have yet another place that atomizes strings without zone, in Gecko side.

https://searchfox.org/mozilla-central/rev/8d108a59d067ce37671090b0b1972ee8adfb7196/js/xpconnect/src/XPCJSContext.cpp#1410-1413

// static
XPCJSContext* XPCJSContext::NewXPCJSContext() {
  XPCJSContext* self = new XPCJSContext();
  nsresult rv = self->Initialize();

https://searchfox.org/mozilla-central/rev/8d108a59d067ce37671090b0b1972ee8adfb7196/js/xpconnect/src/XPCJSContext.cpp#1387-1388

nsresult XPCJSContext::Initialize() {
...
  nsresult rv =
      CycleCollectedJSContext::Initialize(nullptr, JS::DefaultHeapMaxBytes);
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return rv;
  }

  MOZ_ASSERT(Context());
  JSContext* cx = Context();
...
  MOZ_RELEASE_ASSERT(Runtime()->InitializeStrings(cx),
                     "InitializeStrings failed");

https://searchfox.org/mozilla-central/rev/8d108a59d067ce37671090b0b1972ee8adfb7196/js/xpconnect/src/XPCJSRuntime.cpp#3077-3082

bool XPCJSRuntime::InitializeStrings(JSContext* cx) {
  // if it is our first context then we need to generate our string ids
  if (JSID_IS_VOID(mStrIDs[0])) {
    RootedString str(cx);
    for (unsigned i = 0; i < XPCJSContext::IDX_TOTAL_COUNT; i++) {
      str = JS_AtomizeAndPinString(cx, mStrings[i]);

This could also be true in other embedding...

Comment 10

[Cristina Cozmuta (:CrissCozmuta)]

We also encounter the issue bellow. Can you take a look also at this ?

https://treeherder.mozilla.org/jobs?repo=autoland&duplicate_jobs=visible&resultStatus=testfailed%2Cbusted%2Cexception&revision=7269c2e3b6f8fffd62c5c30bb7a0b72739d68e81&searchStr=Android%2C7.0%2Cx86&selectedTaskRun=Y9r0qkV_Rh-zll-0bCipMw.5

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e23ecacc7e1a
Part 1: Clarify atomization progress in the function name. r=nbp
https://hg.mozilla.org/integration/autoland/rev/008f15149504
Part 2: Move atom-specific path out of string allocation code. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7bb53daa9da3
Part 3: Remove length validation from atom instantiation. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d772c5df1e8f
Part 4: Remove length validation and static string lookup from non-atom string instantiation. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7b575dd3ddc2
Part 5: Optimize well-known symbol construction. r=nbp

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/e23ecacc7e1a
https://hg.mozilla.org/mozilla-central/rev/008f15149504
https://hg.mozilla.org/mozilla-central/rev/7bb53daa9da3
https://hg.mozilla.org/mozilla-central/rev/d772c5df1e8f
https://hg.mozilla.org/mozilla-central/rev/7b575dd3ddc2