1557056 - 0.2 - 0.26% Base Content JS (linux64-shippable, linux64-shippable-qr, osx-10-10-shippable, windows10-64-shippable, windows10-64-shippable-qr, windows7-32-shippable) regression on push bd2ad6987449e5be5014d0e35ccbe599159c9e82 (Wed May 8 2019)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Florin Strugariu [:Bebe]]

We have detected an awsy regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=bd2ad6987449e5be5014d0e35ccbe599159c9e82

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

0% Base Content JS windows7-32-shippable opt 3,224,309.33 -> 3,232,846.67
0% Base Content JS windows10-64-shippable-qr opt 4,131,145.33 -> 4,139,617.33
0% Base Content JS linux64-shippable opt 4,074,757.33 -> 4,083,197.33
0% Base Content JS osx-10-10-shippable opt 4,075,770.67 -> 4,084,184.00
0% Base Content JS linux64-shippable-qr opt 4,074,896.67 -> 4,083,193.33
0% Base Content JS windows10-64-shippable opt 4,131,143.33 -> 4,139,545.33
0% Base Content JS linux64-shippable opt 4,074,722.67 -> 4,082,848.00

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=21312

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/AWSY/Tests

Comment 1

[Steven DeTar [:sdetar]]

Hi Arai, could you take a look at this? It seems the changeset/push referred to by this bug was owned by you.

Comment 2

[Tooru Fujisawa [:arai]]

what are those numbers?

Comment 3

[Eric Rahm [:erahm]]

(In reply to Tooru Fujisawa [:arai] from comment #2)

what are those numbers?

It looks like you regressed JS memory usage by ~10KB when loading about:blank (the awsy ab test), this impacts content process overhead. There are some details on running and links to graphs on the fission memory page.

Comment 4

[Ionuț Goldan [:igoldan]]

(In reply to Tooru Fujisawa [:arai] from comment #2)

what are those numbers?

We have a small flaw in the tool that's generating these reports. I filed bug 1557235 to address this problem.

The more precise regression percentages are:

== Change summary for alert #21312 (as of Wed, 05 Jun 2019 11:40:46 GMT) ==

Regressions:

0.26% Base Content JS windows7-32-shippable opt 3,224,309.33 -> 3,232,846.67
0.21% Base Content JS windows10-64-shippable-qr opt 4,131,145.33 -> 4,139,617.33
0.21% Base Content JS linux64-shippable opt 4,074,757.33 -> 4,083,197.33
0.21% Base Content JS osx-10-10-shippable opt 4,075,770.67 -> 4,084,184.00
0.2% Base Content JS linux64-shippable-qr opt 4,074,896.67 -> 4,083,193.33
0.2% Base Content JS windows10-64-shippable opt 4,131,143.33 -> 4,139,545.33

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21312

Comment 5

[Tooru Fujisawa [:arai]]

It was expected to increase the memory usage.

Before bug 1546232 patch, we weren't holding necessary information (the original function name) in function object in self-hosted global.
bug 1546232 patch changed the AllocKind of those functions [1] from gc::AllocKind::FUNCTION to gc::AllocKind::FUNCTION_EXTENDED,
that has 2 extended slot (2 pointers), per each function object [2].

there are 426 function objects that are the target of this change, in self-hosted global,
and 426 * 2 * sizeof(GCPtrValue) = ~7kB, that would explain the number.

the solutions I can think of are the following:

1. maybe we could add yet another AllocKind for adding only 1 pointer, to reduce the memory consumption, given we only use 1 slot,
   but I'm not sure how feasible it is, in term of complexity.

2. the slot is necessary only for functions that cannot use canonical name in original declaration, and _SetCanonicalName is called later [3],
   and only 16 functions needed the slot [4].
   so, we could keep using gc::AllocKind::FUNCTION for others to reduce the memory consumption.
   the issue is that we don't have information about which kind to use, at the point of allocating the function.
   we may need some trick (maybe special function name, or special syntax, or something) to mark them that we're going to call _SetCanonicalName on them,
   or maybe add some special syntax to do them at the same time, without using _SetCanonicalName function.

[1] https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/js/src/gc/AllocKind.h#42-43
[2] https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/js/src/vm/JSFunction.h#978
[3] https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/js/src/builtin/Array.js#792-795
[4] https://searchfox.org/mozilla-central/search?q=_SetCanonicalName&path=js%2Fsrc%2Fbuiltin%2F

Comment 6

[Jeff Walden [:Waldo]]

A special syntax appeals to me from the POV that "self-hosted code is not really JS in all sorts of ways and it is deceptive to pretend otherwise", but other people like having a subset, and being able to lint and other silly things. So I suppose if this tack were followed, a name-prefix or something, recognized at clone time, is about the best you can do.

No particular opinion on the actual question faced here, tho.

Comment 7

[Jan de Mooij [:jandem]]

I was wondering about having a static list like this, as a C++ array-of-structs:

"MapEntries" -> "entries"
"RegExpFlagsGetter" -> "get flags"
...

Maybe encoding an index or so in the function name would be faster though...

Comment 8

[Tooru Fujisawa [:arai]]

WIP with special function name for FUNCTION_EXTENDED kind here
https://hg.mozilla.org/try/rev/cf144eab4f5b4d9952ca984e6d2089eb1872b70f

will try the static list as well.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #8)

WIP with special function name for FUNCTION_EXTENDED kind here
https://hg.mozilla.org/try/rev/cf144eab4f5b4d9952ca984e6d2089eb1872b70f

will try the static list as well.

FWIW I think your patch is nicer than having a separate list :) A separate list is less explicit and it's easy to forget about.

Comment 10

[Tooru Fujisawa [:arai]]

There's an issue that I'm not sure how to solve.
I've added JSFunction::isExtended() call in helper thread, and it's considered to be a race, against JSFunction::setUnlazifiedScript in main thread, both touches JSFunction.flags_ field, but that's different bit.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=250553241&repo=try&lineNumber=14432

Comment 11

[Jeff Walden [:Waldo]]

Data races are tracked at essentially the byte level. Distinct bit-fields are not separate locations for data-race purposes. Nor are distinct bits within a single byte stored as uint8_t or so -- operations to perform bit-sets and the like are not atomic unless they use actual atomic operations to do it. (And I'm guessing making a flags byte fully atomic is not a viable solution.)

Off the top of my head I have no solution to offer, just drive-by memory-model kibitzing.

Comment 12

[Tooru Fujisawa [:arai]]

Thank you for the info :)

I was wondering if we can split flags_ into 2 fields, one for static things (set at creating, never change), and one for dynamic things (can be modified multiple times),
but unfortunately there are 10 static bits (7 flags, 3bits for function kind) and 6 dynamic bits (6 flags), so there's not much clean solution :/

Comment 13

[Jan de Mooij [:jandem]]

In IsRegExpHoistableCall we could use the WrappedFunction for this. Maybe add an isExtended bit to it?

Comment 14

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #13)

In IsRegExpHoistableCall we could use the WrappedFunction for this. Maybe add an isExtended bit to it?

Thanks!
I'll try implementing it.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1557056 - Use extended function only for self-hosted function that needs to store canonical name in extended slot. r?jandem

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dab3163234b5
Use extended function only for self-hosted function that needs to store canonical name in extended slot. r=jandem

Comment 17

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/dab3163234b5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta backport? Looks like a big patch, but mostly just find & replace?

Comment 19

[Tooru Fujisawa [:arai]]

I'll prepare patch.

Comment 20

[Tooru Fujisawa [:arai]]

Comment on attachment 9072079 [details]
Bug 1557056 - Use extended function only for self-hosted function that needs to store canonical name in extended slot. r?jandem

Beta/Release Uplift Approval Request

• User impact if declined: increased memory consumption (see comment #0)
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=autoland,1959153,1,4&zoom=1560822893359.1252,1561088330318.7554,3140955.052750834,3275000&selected=autoland,1959153,498457,841554294,4

on Base Content JS windows7-32-shippable opt

regression (in comment #0):
3,224,309.33 -> 3,232,846.67 (+8537)
fix (above link):
3,167,728.00 -> 3,159,664.00 (-8064)

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This removes extra allocation of JS objects (allocating larger object than necessary).
• String changes made/needed:

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9072079 [details]
Bug 1557056 - Use extended function only for self-hosted function that needs to store canonical name in extended slot. r?jandem

I was a bit on the fence with this one since it doesn't seem such a severe regression to me. Vicky Chin reviewed it and her opinion is to take the fix and not ship with this regression. Given that, Beta68+

Comment 22

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8fb74fcbc12c

Comment 23

[Florin Strugariu [:Bebe]]

== Change summary for alert #21683 (as of Mon, 01 Jul 2019 04:42:28 GMT) ==

Improvements:

0.26% Base Content JS windows7-32-shippable opt 3,167,840.00 -> 3,159,610.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21683