Closed Bug 1226936 Opened 8 years ago Closed 8 years ago

Remove PreserveRegExpStatics.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(2 files)

(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
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.
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
Assignee: nobody → arai.unmht
Attachment #8691209 - Flags: review?(till)
Comment on attachment 8691209 [details] [diff] [review]
Remove PreserveRegExpStatics and telemetry for it.

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

So nice!
Attachment #8691209 - Flags: review?(till) → review+
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?
Attachment #8691209 - Flags: review?(benjamin)
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.
(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?
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
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
>     };
Attachment #8691371 - Flags: review?(benjamin)
Attachment #8691371 - Flags: review?(benjamin) → review+
Attachment #8691209 - Flags: review?(benjamin)
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
https://hg.mozilla.org/mozilla-central/rev/880bab4fbe0a
https://hg.mozilla.org/mozilla-central/rev/467c39dee882
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.