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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[André Bargull [:anba]]

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

Comment 1

[André Bargull [:anba]]

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!

Comment 2

[André Bargull [:anba]]

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

Comment 3

[André Bargull [:anba]]

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

Comment 4

[Till Schneidereit [: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.

Comment 5

[Till Schneidereit [:till]]

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.

Comment 6

[André Bargull [:anba]]

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

Comment 7

[André Bargull [:anba]]

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

Comment 8

[André Bargull [:anba]]

Attachment: bug1387400-part1-elembase-large-strings.patch

Update string limit to 5k per comment #5. Carrying r+.

Comment 9

[André Bargull [:anba]]

Attachment: bug1387400-part2-pass-flags.patch

Rebased part 2 to apply on the updated part 1. Carrying r+.

Comment 10

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06f70a56db4dbb89f79f68615a7d8817c6357b4a

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b88c60c64249
https://hg.mozilla.org/mozilla-central/rev/3167f8853dff