Support GetElemBaseForLambda in Ion resp. only call GetElemBaseForLambda for long strings

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
15 days ago
9 days ago

People

(Reporter: André Bargull, Assigned: André Bargull)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

15 days ago
Created attachment 8893779 [details] [diff] [review]
bug-inline-getelembase.patch

Per bug 1365361, GetElemBaseForLambda is called over a million times in Speedometer, so I thought it'd be nice to have at least basic Ion-support for this function. This basic support worked out quite nicely for AngularJS (calls dropped from 11K to 2K for a single run of AngularJS), but didn't show any improvements for React and React-Redux (25K resp. 20K calls to GetElemBaseForLambda). Turns out that React uses this function, which exactly matches the pattern GetElemBaseForLambda looks for (http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/js/src/builtin/RegExp.cpp#1684-1690):

function escape(key) {
  var escapeRegex = /[=:]/g;
  var escaperLookup = {
    '=': '=0',
    ':': '=2'
  };
  var escapedString = ('' + key).replace(escapeRegex, function (match) {
    return escaperLookup[match];
  });

  return '$' + escapedString;
}


So instead of trying to inline (or in addition to inlining, depending on whether or not we want to take this patch), maybe we should just disable the getElemBaseForLambda call in http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/js/src/builtin/RegExp.js#288-292 when the input string is small (<100, <1000, <10000 character?), because this optimization is really only there for packed source strings from the Dean Edwards packers or other similar packers.




For reference, here are the calls to js::intrinsic_GetElemBaseForLambda when running each framework in Speedometer once:

Vanilla             0
Vanilla2015         0
Vanilla-babel       0
React               24950
React-Redux         20897
Ember               79
Backbone            722
AngularJS           11001
Angular2            0
Vue                 1
jQuery              1518
Preact              0
Inferno             0
Elm                 0
Flight              228
(Assignee)

Comment 1

15 days ago
Oh, and I forgot to mention that the replacer function in React's escape function doesn't seem to be called at all in Speedometer, which means we don't get anything from calling GetElemBaseForLambda in this case!
(Assignee)

Comment 2

12 days ago
Created attachment 8894436 [details] [diff] [review]
bug1387400-part1-elembase-large-strings.patch

This patch removes all GetElemBaseForLambda calls in Speedometer by adding a minimum required length before trying the elem-base optimization. We could bump the limit to 10000 characters instead of 1000, but I decided to be conservative a use the lower limit for now (all strings in ss-string-unpack-code (*) are larger than 10000 characters). 


(*) The elem-base optimization was only re-added in bug 1264264 for ss-string-unpack-code. Maybe we should remeasure if it's still necessary to keep this optimization at all...
Assignee: nobody → andrebargull
Attachment #8893779 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8894436 - Flags: review?(till)
(Assignee)

Comment 3

12 days ago
Created attachment 8894438 [details] [diff] [review]
bug1387400-part2-pass-flags.patch

And another small optimization for global replacements with a replacer function:
Instead of reading the REGEXP_FLAGS_SLOT again in RegExpGlobalReplaceOptFunc/RegExpGlobalReplaceOptElemBase, we can simply pass the |flags| argument directly. This patch doesn't make much of a difference in ion-code, because UnsafeGetInt32FromReservedSlot is (or should generally be) inlined, but I figured it doesn't harm to make this change.
Attachment #8894438 - Flags: review?(till)
Comment on attachment 8894438 [details] [diff] [review]
bug1387400-part2-pass-flags.patch

Review of attachment 8894438 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, r=me.

// UnsafeGetInt32FromReservedSlot is (or should generally be) inlined

This is actually interesting: I wonder if we sometimes fail to inline these intrinsics because we lack type information. We should always be able to inline them based on assumptions that the intrinsic asserts anyway, so if we sometimes don't, we should probably try to change that. Obviously not fodder for this bug.
Attachment #8894438 - Flags: review?(till) → review+
Comment on attachment 8894436 [details] [diff] [review]
bug1387400-part1-elembase-large-strings.patch

Review of attachment 8894436 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense. I wouldn't be opposed to being slightly more aggressive - say using 5k instead of 1k, but this is certainly fine as-is. r=me with either value.
Attachment #8894436 - Flags: review?(till) → review+
(Assignee)

Comment 6

11 days ago
(In reply to Till Schneidereit [:till] from comment #4)
> // UnsafeGetInt32FromReservedSlot is (or should generally be) inlined
> 
> This is actually interesting: I wonder if we sometimes fail to inline these
> intrinsics because we lack type information. We should always be able to
> inline them based on assumptions that the intrinsic asserts anyway, so if we
> sometimes don't, we should probably try to change that. Obviously not fodder
> for this bug.

As part of bug 1365361 we've already identified a few intrinsics which didn't get inlined because of type information issues (bug 1383645, bug 1366263, bug 1383643, bug 1383644). \o/
(Assignee)

Comment 7

11 days ago
(In reply to Till Schneidereit [:till] from comment #5)
> Comment on attachment 8894436 [details] [diff] [review]
> bug1387400-part1-elembase-large-strings.patch
> 
> Review of attachment 8894436 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Makes sense. I wouldn't be opposed to being slightly more aggressive - say
> using 5k instead of 1k, but this is certainly fine as-is. r=me with either
> value.

I'll change it to 5k.
(Assignee)

Comment 8

10 days ago
Created attachment 8895342 [details] [diff] [review]
bug1387400-part1-elembase-large-strings.patch

Update string limit to 5k per comment #5. Carrying r+.
Attachment #8894436 - Attachment is obsolete: true
Attachment #8895342 - Flags: review+
(Assignee)

Comment 9

10 days ago
Created attachment 8895343 [details] [diff] [review]
bug1387400-part2-pass-flags.patch

Rebased part 2 to apply on the updated part 1. Carrying r+.
Attachment #8894438 - Attachment is obsolete: true
Attachment #8895343 - Flags: review+
(Assignee)

Comment 10

10 days ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06f70a56db4dbb89f79f68615a7d8817c6357b4a
Keywords: checkin-needed

Comment 11

9 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b88c60c64249
Part 1: Only call GetElemBaseForLambda for large strings. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/3167f8853dff
Part 2: Pass the original regexp flags to the specializer replacer functions. r=till
Keywords: checkin-needed

Comment 12

9 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b88c60c64249
https://hg.mozilla.org/mozilla-central/rev/3167f8853dff
Status: ASSIGNED → RESOLVED
Last Resolved: 9 days ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.