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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Josh Mize [:jgmize]]

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.

Comment 1

[Tooru Fujisawa [:arai]]

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.

Comment 2

[Josh Mize [:jgmize]]

(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.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Add telemetry for regexp.hasOwnProperty("source") and Object.getOwnPropertyDescriptor(regexp, "source").

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?

Comment 4

[Till Schneidereit [:till]]

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).

Comment 5

[Till Schneidereit [:till]]

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 6

[Josh Mize [:jgmize]]

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 :)

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Add telemetry for regexp.hasOwnProperty("source") and Object.getOwnPropertyDescriptor(regexp, "source") on non-release build.

Thank you all :D
Added `#ifndef RELEASE_BUILD` for each change.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d72b5ae4f4c

Comment 8

[Till Schneidereit [: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!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef334b744581

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef334b744581