Remove PreserveRegExpStatics.

RESOLVED FIXED in Firefox 45

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
(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.
(Assignee)

Comment 2

a year ago
Created attachment 8691209 [details] [diff] [review]
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
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.
(Assignee)

Comment 6

a year ago
(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)
(Assignee)

Comment 8

a year ago
Created attachment 8691371 [details] [diff] [review]
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
>     };
Attachment #8691371 - Flags: review?(benjamin)
Attachment #8691371 - Flags: review?(benjamin) → review+
Attachment #8691209 - Flags: review?(benjamin)
(Assignee)

Comment 9

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/880bab4fbe0a
https://hg.mozilla.org/mozilla-central/rev/467c39dee882
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.