Closed Bug 1557056 Opened 4 months ago Closed 4 months ago

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)

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: Bebe, Assigned: arai)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: perf, regression)

Attachments

(1 file)

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

Component: General → JavaScript Engine
Product: Testing → Core
Version: Version 3 → unspecified
Blocks: 1549347
Regressed by: 1546232

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

Flags: needinfo?(arai.unmht)
Priority: -- → P2

what are those numbers?

Flags: needinfo?(fstrugariu)

(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.

(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

Flags: needinfo?(fstrugariu)

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

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.

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...

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

will try the static list as well.

(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.

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

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.

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 :/

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

(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.

Flags: needinfo?(arai.unmht)
Flags: needinfo?(arai.unmht)
Pushed by arai_a@mac.com:
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
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Assignee: nobody → arai.unmht

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

I'll prepare patch.

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

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:
Flags: needinfo?(arai.unmht)
Attachment #9072079 - Flags: approval-mozilla-beta?

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+

Attachment #9072079 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

== 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

You need to log in before you can comment on or make changes to this bug.