Closed Bug 1153963 Opened 9 years ago Closed 9 years ago

Add telemetry probe for accessing RegExp#source as an instance property

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jgmize, Assigned: arai)

References

Details

Attachments

(1 file, 1 obsolete file)

As suggested by :hallvors in https://bugzilla.mozilla.org/show_bug.cgi?id=1138325#c13 we should measure the potential impact of turning RegExp#source from an instance property into an accessor, as originally implemented in bug 1120169 and backed out in bug 1150297.
See Also: → 1138325, 1150297
Summary: Add telemetry probe for acessing RegExp#source as an instance property → Add telemetry probe for accessing RegExp#source as an instance property
What kind of operation do we actually want to measure?
 1. regexp.source
 2. v = "source"; regexp[v]
 3. regexp.hasOwnProperty("source")
 4. Object.getOwnPropertyDescriptor(regexp, "source")
 5. something else?

To detect ColojureScript's case, I guess 3 should be sufficient (fixed version uses `regexp instanceof RegExp`),
  https://github.com/clojure/clojurescript/commit/d717b4edea074fcfd3e718a6134238ba26f76f82
and 1 and 2 won't matter since they works well on both ES5 and ES6 cases.
not sure whether 4 is used in practice.
(In reply to Tooru Fujisawa [:arai] from comment #1)
> What kind of operation do we actually want to measure?
>  1. regexp.source
>  2. v = "source"; regexp[v]
>  3. regexp.hasOwnProperty("source")
>  4. Object.getOwnPropertyDescriptor(regexp, "source")
>  5. something else?
> 
> To detect ColojureScript's case, I guess 3 should be sufficient (fixed
> version uses `regexp instanceof RegExp`),
>  
> https://github.com/clojure/clojurescript/commit/
> d717b4edea074fcfd3e718a6134238ba26f76f82
> and 1 and 2 won't matter since they works well on both ES5 and ES6 cases.
> not sure whether 4 is used in practice.

Great question, :arai. I'm not sure whether 4 is used in practice either, and that hadn't even occurred to me before now. While 3 is probably sufficient, it would be nice to have data for all cases that are affected by the change, if only to support the hypothesis of their rarity. I agree that 1 and 2 should be excluded from the telemetry data collection since they are not affected by the change.
OS: Linux → All
Hardware: x86_64 → All
Added telemetry in Object.prototype.hasOwnProperty and Object.getOwnPropertyDescriptor.
It introduces following performance regressions, are those acceptable range?

Code:
  let t = elapsed();
  let v = {};
  // or let v = /a/;
  for (let i = 0; i < 1000000; i++) {
    v.hasOwnProperty("length");
    // or one of
    //  v.hasOwnProperty("source");
    //  Object.getOwnPropertyDescriptor(v, "length");
    //  Object.getOwnPropertyDescriptor(v, "source");
  }
  print(elapsed() - t);

Result:
                    | before [us] | after [us] | time
--------------------+-------------+------------+--------------------------
object + has length | 22875.6     | 23875.2    | 1.04x
object + has source | 22843.4     | 23697.2    | 1.04x
object + get length | 32418.7     | 31669.4    | 0.98x (what happens ...?)
object + get source | 32639.1     | 31501.4    | 0.97x (again !?)
regexp + has length | 24362.2     | 24888.5    | 1.02x
regexp + has source | 24478.0     | 46965.2    | 1.92x
regexp + get length | 33192.6     | 33263.4    | 1.01x
regexp + get source | 33360.0     | 51295.4    | 1.54x

*has: v.hasOwnProperty(prop)
*get: Object.getOwnPropertyDescriptor(v, prop)


Also, is it better to measure hasOwnProperty and getOwnPropertyDescriptor separately?
Attachment #8594070 - Flags: feedback?(till)
Attachment #8594070 - Flags: feedback?(jmize)
Comment on attachment 8594070 [details] [diff] [review]
Add telemetry for regexp.hasOwnProperty("source") and Object.getOwnPropertyDescriptor(regexp, "source").

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

I think that this is fine for Nightly, at least, perhaps also Developer Edition. The impact seems negligible for everything but the source property itself, after all.

So, f+ from me with an #ifdef making it active only in non-release builds (i.e., Nightly and Developer Edition).
Attachment #8594070 - Flags: feedback?(till) → feedback+
Oh, and I'm pretty sure that the changes for other property names you saw are mostly noise. I don't know how many times you ran your benchmark, but it seems likely to me that multiple runs would give different results.

> Also, is it better to measure hasOwnProperty and getOwnPropertyDescriptor separately?

I don't think so. The effect for our purposes here is the same, after all.
Comment on attachment 8594070 [details] [diff] [review]
Add telemetry for regexp.hasOwnProperty("source") and Object.getOwnPropertyDescriptor(regexp, "source").

+1 to everything :till said, and a big thank you from me to both of you :)
Attachment #8594070 - Flags: feedback?(jmize) → feedback+
Thank you all :D
Added `#ifndef RELEASE_BUILD` for each change.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d72b5ae4f4c
Assignee: nobody → arai.unmht
Attachment #8594070 - Attachment is obsolete: true
Attachment #8594902 - Flags: review?(till)
Comment on attachment 8594902 [details] [diff] [review]
Add telemetry for regexp.hasOwnProperty("source") and Object.getOwnPropertyDescriptor(regexp, "source") on non-release build.

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

Most excellent, thanks!
Attachment #8594902 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/ef334b744581
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1160986
You need to log in before you can comment on or make changes to this bug.