1486885 - Error logging for getBaseDomain should probably be removed; also doesn't work

Status: RESOLVED FIXED

(Web Compatibility :: Interventions, defect, P1)

Description

[:Gijs (he/him)]

This snippet:

    try {
      return Services.eTLD.getBaseDomain(uri);
    } catch (ex) {
      console.error(`Could not getBaseDomain() for "${uri}"`, ex);
      return false;
    }


Logs

 0:05.17 GECKO(3716) console.error: "Could not getBaseDomain() for \"[xpconnect wrapped nsIURI]\"" ({})

e.g. when running browser/base/content/test/plugins/browser_private_clicktoplay.js .

It should probably log `uri.spec`.

But also, this not working is expected for domain-less URLs (e.g. about: URLs) , and we probably shouldn't log errors for this.

Comment 1

[Dennis Schubert [:denschub]]

Ah, thanks for the note, Gijs. I did some reading and decided we probably don't need the exception logging in any case, so I removed it. The code is currently waiting internal review, and I'll land it as soon as possible. :)

Comment 2

[Dennis Schubert [:denschub]]

Attachment: Bug 1486885 - Remove error logging inside getBaseDomainFromURI(). r=gijs

Comment 3

[Dennis Schubert [:denschub]]

Gijs, as you found the error, I hope you don't mind if I r? you. :)

Comment 4

[:Gijs (he/him)]

Comment on attachment 9005236 [details]
Bug 1486885 - Remove error logging inside getBaseDomainFromURI(). r=gijs

:Gijs (he/him) has approved the revision.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/396e4cc1baef
Remove error logging inside getBaseDomainFromURI(). r=Gijs

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/396e4cc1baef

Comment 7

[Ionuț Goldan [:igoldan]]

Big perf wins:

== Change summary for alert #15523 (as of Thu, 30 Aug 2018 12:51:56 GMT) ==

Improvements:

 21%  tp5o responsiveness windows7-32 pgo e10s stylo     0.53 -> 0.42
 20%  tp5o responsiveness windows10-64 pgo e10s stylo    0.56 -> 0.45
 17%  tp5o responsiveness windows10-64-qr opt e10s stylo 0.61 -> 0.51
 15%  tp5o responsiveness windows10-64 opt e10s stylo    0.60 -> 0.51
 14%  tp5o responsiveness linux64 pgo e10s stylo         0.74 -> 0.63
 14%  tp5o responsiveness windows7-32 opt e10s stylo     0.56 -> 0.49
 12%  tp5o_webext responsiveness windows10-64 pgo e10s stylo1.15 -> 1.01
 12%  tp5o_webext responsiveness windows7-32 pgo e10s stylo1.05 -> 0.93
 11%  tp5o_webext responsiveness windows7-32 opt e10s stylo1.13 -> 1.00
 10%  tp5o_webext responsiveness windows10-64 opt e10s stylo1.23 -> 1.10
 10%  tp5o_webext responsiveness linux64 opt e10s stylo  1.50 -> 1.34
 10%  tp5o_webext responsiveness windows10-64-qr opt e10s stylo1.25 -> 1.12
 10%  tp5o_webext responsiveness linux64 pgo e10s stylo  1.35 -> 1.22

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=15523

Comment 8

[:Gijs (he/him)]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #7)
> Big perf wins:
> 
> == Change summary for alert #15523 (as of Thu, 30 Aug 2018 12:51:56 GMT) ==
> 
> Improvements:
> 
>  21%  tp5o responsiveness windows7-32 pgo e10s stylo     0.53 -> 0.42
>  20%  tp5o responsiveness windows10-64 pgo e10s stylo    0.56 -> 0.45
>  17%  tp5o responsiveness windows10-64-qr opt e10s stylo 0.61 -> 0.51
>  15%  tp5o responsiveness windows10-64 opt e10s stylo    0.60 -> 0.51
>  14%  tp5o responsiveness linux64 pgo e10s stylo         0.74 -> 0.63
>  14%  tp5o responsiveness windows7-32 opt e10s stylo     0.56 -> 0.49
>  12%  tp5o_webext responsiveness windows10-64 pgo e10s stylo1.15 -> 1.01
>  12%  tp5o_webext responsiveness windows7-32 pgo e10s stylo1.05 -> 0.93
>  11%  tp5o_webext responsiveness windows7-32 opt e10s stylo1.13 -> 1.00
>  10%  tp5o_webext responsiveness windows10-64 opt e10s stylo1.23 -> 1.10
>  10%  tp5o_webext responsiveness linux64 opt e10s stylo  1.50 -> 1.34
>  10%  tp5o_webext responsiveness windows10-64-qr opt e10s stylo1.25 -> 1.12
>  10%  tp5o_webext responsiveness linux64 pgo e10s stylo  1.35 -> 1.22
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=15523

Uhhh... this really shouldn't have made a difference.

I wonder if the domain checks are expensive in tp5o, and if this basically offsets the regression in bug 1484633 (and if not, if there are other console.log/error things that we may also need to remove?).

Kris, do you know if the former is possible here, and Ionuț, can you check the latter?

Comment 9

[Ionuț Goldan [:igoldan]]

(In reply to :Gijs (he/him) from comment #8)
> (In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #7)
> > Big perf wins:
> > 
> > == Change summary for alert #15523 (as of Thu, 30 Aug 2018 12:51:56 GMT) ==
> > 
> > Improvements:
> > 
> >  21%  tp5o responsiveness windows7-32 pgo e10s stylo     0.53 -> 0.42
> >  20%  tp5o responsiveness windows10-64 pgo e10s stylo    0.56 -> 0.45
> >  17%  tp5o responsiveness windows10-64-qr opt e10s stylo 0.61 -> 0.51
> >  15%  tp5o responsiveness windows10-64 opt e10s stylo    0.60 -> 0.51
> >  14%  tp5o responsiveness linux64 pgo e10s stylo         0.74 -> 0.63
> >  14%  tp5o responsiveness windows7-32 opt e10s stylo     0.56 -> 0.49
> >  12%  tp5o_webext responsiveness windows10-64 pgo e10s stylo1.15 -> 1.01
> >  12%  tp5o_webext responsiveness windows7-32 pgo e10s stylo1.05 -> 0.93
> >  11%  tp5o_webext responsiveness windows7-32 opt e10s stylo1.13 -> 1.00
> >  10%  tp5o_webext responsiveness windows10-64 opt e10s stylo1.23 -> 1.10
> >  10%  tp5o_webext responsiveness linux64 opt e10s stylo  1.50 -> 1.34
> >  10%  tp5o_webext responsiveness windows10-64-qr opt e10s stylo1.25 -> 1.12
> >  10%  tp5o_webext responsiveness linux64 pgo e10s stylo  1.35 -> 1.22
> > 
> > For up to date results, see:
> > https://treeherder.mozilla.org/perf.html#/alerts?id=15523
> 
> Uhhh... this really shouldn't have made a difference.
> 
> I wonder if the domain checks are expensive in tp5o, and if this basically
> offsets the regression in bug 1484633 (and if not, if there are other
> console.log/error things that we may also need to remove?).

The tp5o responsiveness tests have returned to values prior to bug 1484633. tp5o have returned almost entirely: they're still just slightly regressed than values prior to bug 1484633.

Comment 10

[Kris Maglione [:kmag]]

(In reply to :Gijs (he/him) from comment #8)
> Uhhh... this really shouldn't have made a difference.
> 
> I wonder if the domain checks are expensive in tp5o, and if this basically
> offsets the regression in bug 1484633 (and if not, if there are other
> console.log/error things that we may also need to remove?).
> 
> Kris, do you know if the former is possible here, and Ionuț, can you check
> the latter?

So, a few things:

1) I don't really trust tp5o responsiveness at this point. It definitely measures *something*, but at the current baseline numbers, I'm not sure what that something is, and I'm not sure it's actually useful at this point.

2) XPConnect stringifiers are expensive, and that template string always triggers them. Console logging is also pretty expensive. An just touching the console object causes us to create it and its prototypes if they haven't already been created. So if we're actually catching errors here, I'd expect them to add significant overhead.

3) Yes, if there are other console logs we can remove from hot code paths, we probably should.

4) ... Domain checks probably are expensive, but I don't think that's relevant here, since we didn't remove any of them. We just removed the error logging for failures.

Comment 11

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #10)
> 3) Yes, if there are other console logs we can remove from hot code paths,
> we probably should.

From a very cursory audit, I didn't see something that looked like it'd be hot, though our sync code seems to love console.* with template strings, we use console.log a bunch in mobile browser.js (which seems pretty pointless), and our dom Push code is littered with it. But none of that seems hot, though mobile browser.js seems like a poor place for it, so I filed bug 1489458.