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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
Posted patch bug-inline-getelembase.patch (obsolete) — Splinter Review
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

2 years 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

2 years ago
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

2 years ago
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

2 years 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

2 years 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

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

Comment 9

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

Comment 11

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b88c60c64249
https://hg.mozilla.org/mozilla-central/rev/3167f8853dff
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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.