1130123 - Add telemetry for __noSuchMethod__

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

We want to remove __noSuchMethod__, as it's non-standard, complicates a lot of code and is probably broken in a lot of places in the JITs.

As a first step we should probably add telemetry for calls to js::OnUnknownMethod (js/src/vm/Interpreter.cpp).

Chris, would you mind taking this?

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> As a first step we should probably add telemetry for calls to
> js::OnUnknownMethod (js/src/vm/Interpreter.cpp).

To be clear, we only care about the "if (value.isObject())" branch there. Or we could add it to the NoSuchMethod function below it.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: noSuchMethod-telemetry.patch

Jan, our current telemetry for nonstandard language extensions is recorded at parse-time, so we can report just one hit per parse ("page-view") in Parser's destructor. I don't think we care if a website uses a nonstandard language extension once or 1000x. We want to know whether we're going to break one website or many.

1. This patch counts every __noSuchMethod__ call. If we want to only report one hit per page-view, where do you recommend I record a boolean flag that __noSuchMethod__ has been called at least once?

2. Also, our current parser telemetry only reports for web content, not add-on or chrome JS. It checks whether the current compartment has an addonId and whether the parsed JS has a filename beginning with "http[s]://". Is this relevant for __noSuchMethod__? Does Interpreter.cpp have access to the parsed file's filename?

https://hg.mozilla.org/mozilla-central/file/tip/js/src/frontend/Parser.cpp#l8383

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8560965 [details] [diff] [review]
noSuchMethod-telemetry.patch

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

Thanks for doing this!

(In reply to Chris Peterson [:cpeterson] from comment #2)
> 1. This patch counts every __noSuchMethod__ call. If we want to only report
> one hit per page-view, where do you recommend I record a boolean flag that
> __noSuchMethod__ has been called at least once?

I'd add a bool to JSCompartment (jscompartment.h).

> 2. Also, our current parser telemetry only reports for web content, not
> add-on or chrome JS. It checks whether the current compartment has an
> addonId and whether the parsed JS has a filename beginning with
> "http[s]://". Is this relevant for __noSuchMethod__? Does Interpreter.cpp
> have access to the parsed file's filename?

You can do something like this:

if (JSScript *script = cx->currentScript()) {
    const char *filename = script->filename();
    ...
}

(Note that the 'console' object currently uses __noSuchMethod__, to silently accept console.foo() etc. This is one of the very few uses in our code base and I think we can fix that, but it might affect the telemetry results..)

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Chris Peterson [:cpeterson] from comment #2)
> 2. Also, our current parser telemetry only reports for web content, not
> add-on or chrome JS. It checks whether the current compartment has an
> addonId and whether the parsed JS has a filename beginning with
> "http[s]://". Is this relevant for __noSuchMethod__?

I think it makes sense to match the parser telemetry, at least for now, so that we can compare the results. I'm hoping that we'll get very few hits in web content, especially once we fix the console object...

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> I think it makes sense to match the parser telemetry, at least for now, so
> that we can compare the results. I'm hoping that we'll get very few hits in
> web content, especially once we fix the console object...

Should the console object be fixed before landing this telemetry probe?

Should we also log a console warning that __noSuchMethod__ is deprecated? I could do that in a follow-up bug.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Chris Peterson [:cpeterson] from comment #5)
> Should the console object be fixed before landing this telemetry probe?

I don't think that's necessary. We can always compare the numbers we get before/after we fix the console.

> Should we also log a console warning that __noSuchMethod__ is deprecated? I
> could do that in a follow-up bug.

Thanks, that'd be awesome. We should fix the console (and other internal uses) first though, I can hopefully look into that soon.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> We should fix the console (and other internal
> uses) first though, I can hopefully look into that soon.

Of course, anybody please feel free to beat me to this :)

I also wonder if we should do a dev-platform post with a list of deprecated JS features, so people know they should stop using them.

Comment 8

[Florian Scholz (Open Web Docs)]

> I also wonder if we should do a dev-platform post with a list of deprecated
> JS features, so people know they should stop using them.

I added __noSuchMethod__ to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Deprecated_and_obsolete_features, which I encourage you to keep up to date, too :)

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: part-1-refactor-telemetry.patch

Part 1: Move deprecated JS telemetry from Parser to JSCompartment. This change refactors the telemetry code so it will be accessible from Interpreter.cpp. The trivial __noSuchMethod__ change is in patch 2.

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: part-2-noSuchMethod-telemetry.patch

Part 2: Add telemetry for __noSuchMethod__. This patch avoids reporting __noSuchMethod__ telemetry for the console object.

I should note that part 1 changed the chrome JS detection to check compartment()->isSystem instead of looking for script filenames not beginning with "http[s]://". AFAICT, these checks have the same result.

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: part-2-noSuchMethod-telemetry-v2.patch

oops. I forgot to add the new telemetry probe description to Histograms.json.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8562584 [details] [diff] [review]
part-1-refactor-telemetry.patch

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

Nice refactoring! This should make it easier to add telemetry for other JS features.

::: js/src/jscompartment.cpp
@@ +821,5 @@
> +void
> +JSCompartment::reportTelemetry()
> +{
> +    MOZ_ASSERT(!addonId && !isSystem,
> +               "Only report telemetry for web content, not add-ons or chrome JS.");

Why does this assert hold? ~JSCompartment calls reportTelemetry() unconditionally, so I think it should be an early return:

// Only report telemetry for web content, not add-ons or chrome JS.
if (addonId || isSystem)
   return;

@@ +829,5 @@
> +
> +    // Call back into Firefox's Telemetry reporter.
> +    for (size_t i = 0; i < DeprecatedLanguageExtensionCount; i++) {
> +        if (sawDeprecatedLanguageExtension[i]) {
> +                                                                                                            printf("XXX reportTelemetry: saw %zu\n", i);

Nit: remove the printf. Also, the call below should fit on one line (max 99 chars for code, 80 for comments in SpiderMonkey code), and then you can drop the {} (single-line if-body).

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8562595 [details] [diff] [review]
part-2-noSuchMethod-telemetry-v2.patch

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

Awesome, thanks!

Comment 14

[Chris Peterson [:cpeterson]]

Thanks! I made the changes you suggested and removed the debug printf I accidentally left in the patch. <:)

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6cc119cedd
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8356c3884b2

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb6cc119cedd
https://hg.mozilla.org/mozilla-central/rev/c8356c3884b2

Comment 16

[Chris Peterson [:cpeterson]]

Jan, the telemetry probe for __noSuchMethod__ landed about four days ago and we have some initial results. Unfortunately, either __noSuchMethod__ is used a surprising amount (called in 65K compartment over four days) or my refactoring is inadvertently including addon or chrome JS usage.

If you compare Nightly 38 and Aurora 37 for the same time period, the telemetry measurements are roughly on the same order of magnitude *except* for-each-in loops:

Nightly 38 telemetry: http://is.gd/VEyKzH
Aurora 37 telemetry:  http://is.gd/0f4mUm

In Aurora 37 and earlier, I tried to identify web content as scripts with filenames beginning with "http[s]://". In my Nightly 38 refactoring, I changed the code to check JSCompartment's isSystem member variable. Have I misunderstood what isSystem means? Should I add back the "http[s]://" filename check?

Aurora 37 check:
https://hg.mozilla.org/mozilla-central/rev/cb6cc119cedd#l1.189

Nightly 38 check:
https://hg.mozilla.org/mozilla-central/rev/cb6cc119cedd#l3.43

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Chris Peterson [:cpeterson] from comment #16)
> Unfortunately, either __noSuchMethod__ is used
> a surprising amount (called in 65K compartment over four days) or my
> refactoring is inadvertently including addon or chrome JS usage.

This could be the console object, maybe some popular console-method Chrome supports? No idea... Seems a bit unlikely but who knows.

> In Aurora 37 and earlier, I tried to identify web content as scripts with
> filenames beginning with "http[s]://". In my Nightly 38 refactoring, I
> changed the code to check JSCompartment's isSystem member variable. Have I
> misunderstood what isSystem means? Should I add back the "http[s]://"
> filename check?

Local files will probably have file:/// instead of http[s]:// so these might be counted now. Not sure if that's a big fraction.

Maybe addons/chrome can inject/run JS code in the content compartment? Boris, do you have any ideas?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Maybe addons/chrome can inject/run JS code in the content compartment?

Of course they can.  Sometimes they do that, even.

If we just want to detect __noSuchMethod__ on the console, we can try adding a probe specifically for that, right?  Even more interesting would be the property name, but that seems hard.

Comment 19

[Chris Peterson [:cpeterson]]

I think the problem is that the telemetry probe was adding console.whatever() calls (where "whatever" is an unknown property) in .html files to our __noSuchMethod__ telemetry. Even though this is likely a content bug, we don't want to count these calls as __noSuchMethod__ uses.

I will just add back the "http" filename check. Our Nightly 38 telemetry counts have been polluted, but next week's Aurora 38 telemetry should be accurate.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> we don't want to count these calls as __noSuchMethod__ uses.

Why not, exactly?

Comment 21

[Chris Peterson [:cpeterson]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> > we don't want to count these calls as __noSuchMethod__ uses.
> 
> Why not, exactly?

IIUC, Jan wanted to know if web content is using __noSuchMethod__ directly. We know the console object's implementation uses __noSuchMethod__ internally, but we can fix that.

GitHub search shows fewer than 800 hits for __noSuchMethod__ that are not clones of mozilla-central (once you subtract the 3400 hit that are mozilla-central clones). I don't have a exact list because GitHub search doesn't seem to allow the query to exclude filenames (to exclude mozilla-central files).

https://github.com/search?q=__noSuchMethod__+extension%3Ajs+extension%3Ahtml