1226936 - Remove PreserveRegExpStatics.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

(derived from bug 1208835)

PreserveRegExpStatics should be removed for following 2 reasons:
  1. we can simplify the implementation and improve the performance of ES6 compliant version of RegExp.prototype[@@replace] (in bug 887016)
  2. RegExp static property is not a part of ES6 spec and it behaves differently across browsers (bug 1208835 comment #1), so it would be better to reduce the difference between them

Telemetry [1][2] (RestoredRegExpStatics=9) for possibly affected case is added in bug 1208835, and results are following for now:

content
       | Nightly | Aurora
    ---+---------+---------
    44 |     76  |   732
    45 |    275  |     -

Add-ons
       | Nightly | Aurora
    ---+---------+---------
    44 |      0  |     0
    45 |      0  |     -

so, those cases seem to be ignorable.


[1] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-11-20&keys=__none__!__none__!__none__&max_channel_version=nightly%252F45&measure=JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_ADDONS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-10-30&table=1&trim=1&use_submission_date=0
[2] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-11-20&keys=__none__!__none__!__none__&max_channel_version=nightly%252F45&measure=JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-10-29&table=1&trim=1&use_submission_date=0

Comment 1

[Till Schneidereit [:till]]

Excellent! I'm a bit surprised that any content hits this, but at least addons don't, as we expected/hoped.

So yes, let's rip this out, a few hundred hits in content are indeed negligible.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Remove PreserveRegExpStatics and telemetry for it.

Removed PreserveRegExpStatics class, branch in JIT, and telemetry added by bug 1208835.
StrReplaceRegExp was using getRightContext, so changed DoMatchForReplaceGlobal and DoMatchForReplaceLocal to return the index of rightContext.

green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92e19cacb2ac

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8691209 [details] [diff] [review]
Remove PreserveRegExpStatics and telemetry for it.

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

So nice!

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8691209 [details] [diff] [review]
Remove PreserveRegExpStatics and telemetry for it.

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

This removes a probe, so by my reading of https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval, it should be fine to not request a telemetry peer's review. However, I'm not really 100% sure that reading is correct. If it is, could we update the wiki page indicating that removing a probe is fine without review?

Comment 5

[Benjamin Smedberg]

This patch doesn't actually change Histograms.json which still mentions the affected values. You should at least change the docs to say that this value (n=9) is obsolete.

In general I would like to review removals for opt-out (FHR) probes, but removing opt-in (telemetry) probes probably shouldn't require explicit approval.

Comment 6

[Tooru Fujisawa [:arai]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> This patch doesn't actually change Histograms.json which still mentions the
> affected values. You should at least change the docs to say that this value
> (n=9) is obsolete.

What does "docs" here mean?
Adding "(obsolete)" to "description" properties in Histograms.json, or documentation stored somewhere?

Comment 7

[Benjamin Smedberg]

Yes, the description in Histograms.json, which ends up in various dashboards and hopefully soon in the autogenerated docs. Histograms.json is the source of truth which needs to be kept up to date.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Update Histograms.json description to follow jscompartment.h.

Thanks, added "(obsolete)" to all probes that is no longer used (so, 1, 4, 5, 6, 8, and 9).

https://dxr.mozilla.org/mozilla-central/rev/19d89caa664dd9309c796929225ee409df5d1ee4/js/src/jscompartment.h#743
>     enum DeprecatedLanguageExtension {
>         DeprecatedForEach = 0,              // JS 1.6+
>         // NO LONGER USING 1
>         DeprecatedLegacyGenerator = 2,      // JS 1.7+
>         DeprecatedExpressionClosure = 3,    // Added in JS 1.8
>         // NO LONGER USING 4
>         // NO LONGER USING 5
>         // NO LONGER USING 6
>         DeprecatedFlagsArgument = 7,        // JS 1.3 or older
>         // NO LONGER USING 8
>         // NO LONGER USING 9
>         DeprecatedLanguageExtensionCount
>     };

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/880bab4fbe0a1dca32945e7e0e1f4a6e459e108d
Bug 1226936 - Remove PreserveRegExpStatics and telemetry for it. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/467c39dee8822d2e632a71f718d1c325b0dc9c90
Bug 1226936 - Part 2: Update Histograms.json description to follow jscompartment.h. r=bsmedberg

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/880bab4fbe0a
https://hg.mozilla.org/mozilla-central/rev/467c39dee882