1515801 - Minor subscript loader followup fixes after bug 1492937

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Rip out vestigial handling of non-UTF-8 character sets from the subscript loader

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Remove JS::CompileLatin1{ForNonSyntacticScope} now that both are unused

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Make all subscripts loaded by the subscript loader have lazy source, now that potential concerns about inconsistent encodings have been addressed

I am not certain whether this patch is correct.  The comment indicates why I *suspect* it would be.  And callers before -- most of them -- didn't pass in an explicit charset, so they would have been having lazy source before.  But I am not 100% certain of this, so a sanity check would be nice, for the one of these patches that is not exact-functionality-preserving compared to current trunk.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 9032820 [details] [diff] [review]
Rip out vestigial handling of non-UTF-8 character sets from the subscript loader

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

\o/

I think this removes the last remnants of the first patch I ever landed in mozilla-central. But I only cared about making things load in UTF-8 when I landed it, so I guess this is better.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 9032822 [details] [diff] [review]
Make all subscripts loaded by the subscript loader have lazy source, now that potential concerns about inconsistent encodings have been addressed

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

Hmm... I *think* this is OK as long as we still have lazy parsing disabled for the sake of the startup caches. Maybe add a comment that if we enable lazy parsing for some class of scripts, we need to disable lazy source for them.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 9032821 [details] [diff] [review]
Remove JS::CompileLatin1{ForNonSyntacticScope} now that both are unused

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

\o/

Comment 7

[Jeff Walden [:Waldo]]

Try-results coming back indicate that some users -- at a minimum js/xpconnect/tests/chrome/test_chrometoSource.xul in some places -- *depended* on explicitly specifying UTF-8 causing source to be non-lazy.

Also if bug 1515878 isn't a side effect of the laziness-change wrought by happenstance in bug 1492937, I will be very surprised.

So it looks like we need to add a new optional trailing boolean argument to permit forcibly making source be not-lazy (and options-property, for the WithSource varietal).  Gak a little.  Coming up later today, after I do expense-submitting, goal-futzing, survey-filling, and other inanities first...

Comment 8

[Kris Maglione [:kmag]]

(In reply to Jeff Walden [:Waldo] from comment #7)
> Try-results coming back indicate that some users -- at a minimum
> js/xpconnect/tests/chrome/test_chrometoSource.xul in some places --
> *depended* on explicitly specifying UTF-8 causing source to be non-lazy.

Do they? That test is expected to work correctly with lazy source. How exactly is it failing?

Comment 9

[Kris Maglione [:kmag]]

(In reply to Kris Maglione [:kmag] from comment #8)
> (In reply to Jeff Walden [:Waldo] from comment #7)
> > Try-results coming back indicate that some users -- at a minimum
> > js/xpconnect/tests/chrome/test_chrometoSource.xul in some places --
> > *depended* on explicitly specifying UTF-8 causing source to be non-lazy.
> 
> Do they? That test is expected to work correctly with lazy source. How
> exactly is it failing?

It looks like the problem is that the lazy source hook doesn't pass an encoding hint:

https://searchfox.org/mozilla-central/rev/413dd3a1fab3446749f3690d93ee17e3196f8c37/js/xpconnect/src/XPCJSRuntime.cpp#2854

And ConvertToUTF16 falls back to windows-1252 with there's no encoding hint and no BOM:

https://searchfox.org/mozilla-central/rev/413dd3a1fab3446749f3690d93ee17e3196f8c37/js/xpconnect/src/XPCJSRuntime.cpp#2854

Comment 10

[Jeff Walden [:Waldo]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5e196a6e5fc775d93afa364b1ebacb659d16e95&selectedJob=218405780 has a whole mess'o'jobs that fail on that exact test.  (Haven't looked at any of the other failures yet.)  Basically they're ones where we used to have loadSubScript(..., "UTF-8") which dropped us into the non-lazy source path, and then the next line in the text checks toSource() containing certain stuff.

I'll look into comment 9's finer-grained detail shortly, after I finish up doing the reimbursement dance for too many things.

Comment 11

[Jeff Walden [:Waldo]]

Comment 9.  Blah.

I'll try seeing if anything dies just changing that to hint UTF-8.  We're dealing with Gecko-internal stuff now, so if it works, that's good enough.  If anything breaks, it's pretty likely to be breakage of the sort I had to fix for bug 1492937 -- that is, stupid things where we had come to *depend* on malign behavior.  And given how attenuated the path is to that failure case, possibly no one even hits it.

Failing that, while it is possibly not ideal...this is just a test of our own internal making.  We can change it to not fail on this, then just leave an open bug floating around to do a proper fix.  toSource behavior of chrome-code functions is very far from something we consider now as proper API (not since XPI extensions went away and we had to care about third parties monkeypatching up our tabbrowser code by toString and regular expressions).  Arguably we should be incrementally taking source *away* from chrome functions, even.  So if we make it break down some in this one weird case, that does not seem like the end of the world.

But anyway, something for Wednesday -- just wanted to get this off my chest now rather than leave things lurchy-looking going into Christmas...

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Make subscripts have lazy source, and change the source-hook to hint UTF-8 rather than offer no charset hint

So just adding the UTF-8 hint 1) fixes the relevant test, at least on linux64 (test results are all in as I speak, and no oranges that look non-intermittent),

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6a07692488d7e654a1434cabf2266549bffc9f6&group_state=expanded&selectedJob=218854768

and 2) does not currently appear to break anything elsewhere

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a10008517a6b5249bfef4f94c9d1bc1b6c4aff0

(with current results still far from complete) which means I think we can just make that change.  If you disagree, say something and we can figure something out, I guess.  But it would be nice for the adjustment to be this minimal.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Jeff Walden [:Waldo] from comment #11)
> Failing that, while it is possibly not ideal...this is just a test of our
> own internal making.  We can change it to not fail on this, then just leave
> an open bug floating around to do a proper fix.

We really can't. We have a lot of code that depends on being able to stringify functions for things like creating frame scripts. That code already breaks in weird ways if the sources change without the bytecode cache being flushed. If it starts breaking whenever someone inserts a multi-byte character before (or inside) a stringified function, and our source offsets suddenly no longer match what we expect, we're going to have real (and difficult to diagnose) problems.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b504b74f993
Rip out vestigial handling of non-UTF-8 character sets from the subscript loader.  r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/6686393af7c7
Remove JS::CompileLatin1{ForNonSyntacticScope} now that both are unused.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/894e36ecd784
Make subscripts have lazy source, and change the source-hook to hint UTF-8 rather than offer no charset hint so that lazy-source for them will work correctly.  r=kmag

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/8b504b74f993
https://hg.mozilla.org/mozilla-central/rev/6686393af7c7
https://hg.mozilla.org/mozilla-central/rev/894e36ecd784

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

I see perf improvements on startup:
== Change summary for alert #18485 (as of Thu, 27 Dec 2018 14:30:18 GMT) ==

Improvements:

  9%  sessionrestore linux64 opt e10s stylo                          361.17 -> 328.08
  9%  sessionrestore_no_auto_restore linux64 pgo e10s stylo          345.25 -> 314.00
  9%  sessionrestore windows10-64 opt e10s stylo                     361.58 -> 329.50
  9%  sessionrestore_no_auto_restore windows10-64 opt e10s stylo     383.50 -> 349.83
  9%  sessionrestore linux64 pgo e10s stylo                          324.08 -> 295.83
  8%  sessionrestore windows7-32 opt e10s stylo                      349.75 -> 320.08
  8%  sessionrestore_no_auto_restore windows10-64 pgo e10s stylo     359.08 -> 329.00
  8%  sessionrestore windows10-64-qr opt e10s stylo                  359.83 -> 329.92
  8%  sessionrestore windows10-64 pgo e10s stylo                     337.92 -> 310.08
  8%  sessionrestore_no_auto_restore windows7-32 pgo e10s stylo      359.67 -> 331.17
  8%  sessionrestore windows7-32 pgo e10s stylo                      341.83 -> 314.75
  8%  sessionrestore_no_auto_restore linux64 opt e10s stylo          380.67 -> 350.58
  7%  sessionrestore_no_auto_restore windows7-32 opt e10s stylo      367.58 -> 340.58
  7%  ts_paint_webext windows10-64-qr opt e10s stylo                 358.00 -> 333.00
  7%  ts_paint windows7-32 opt e10s stylo                            351.00 -> 326.50
  7%  ts_paint windows10-64-qr opt e10s stylo                        357.67 -> 333.42
  7%  ts_paint_webext windows7-32 opt e10s stylo                     351.21 -> 328.17
  7%  ts_paint_webext windows10-64 opt e10s stylo                    357.50 -> 334.25
  7%  ts_paint windows10-64 opt e10s stylo                           356.58 -> 333.42
  6%  ts_paint windows10-64 pgo e10s stylo                           339.17 -> 317.25
  6%  ts_paint_webext linux64 pgo e10s stylo                         412.38 -> 386.08
  6%  ts_paint_webext windows10-64 pgo e10s stylo                    337.75 -> 316.33
  6%  ts_paint_webext windows7-32 opt e10s stylo                     372.50 -> 349.58
  6%  ts_paint windows7-32 pgo e10s stylo                            344.00 -> 323.08
  6%  ts_paint linux64 opt e10s stylo                                437.79 -> 411.83
  6%  ts_paint_webext windows7-32 pgo e10s stylo                     345.50 -> 325.08
  6%  ts_paint linux64 pgo e10s stylo                                410.21 -> 386.08
  6%  ts_paint windows7-32 opt e10s stylo                            372.17 -> 350.33
  6%  ts_paint_webext linux64 opt e10s stylo                         439.50 -> 414.00
  6%  sessionrestore_no_auto_restore windows7-32 opt e10s stylo      389.92 -> 368.17
  5%  sessionrestore windows7-32 opt e10s stylo                      368.83 -> 350.92
  3%  sessionrestore linux64-qr opt e10s stylo                       819.17 -> 796.31
  3%  ts_paint_webext windows7-32 pgo e10s stylo                     355.26 -> 346.33
  2%  ts_paint windows7-32 pgo e10s stylo                            352.83 -> 344.25
  2%  ts_paint_webext linux64-qr opt e10s stylo                      876.58 -> 857.25
  2%  tp5o_webext windows7-32 opt e10s stylo                         194.82 -> 190.88

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

Comment 17

[Jeff Walden [:Waldo]]

Those changes look like they merely undid the regression reported in bug 1515878.  I will leave it to people there to say otherwise.

Comment 18

[Alexandre Poirot [:ochameau]]

AFAIK I wasn't aware of any significant regression when bug 1515878 landed, but in DevTools we are seeing a couple of significant improvements (we are using loadSubScript for all DevTools modules)

== Change summary for alert #18497 (as of Thu, 27 Dec 2018 02:20:09 GMT) ==

Improvements:

11% damp complicated.netmonitor.open.DAMP windows10-64-qr opt e10s stylo 272.61 -> 241.58
10% damp complicated.netmonitor.open.DAMP windows10-64 pgo e10s stylo 255.39 -> 229.37
10% damp complicated.netmonitor.open.DAMP windows7-32 pgo e10s stylo 252.08 -> 226.98
9% damp complicated.netmonitor.open.DAMP windows7-32 opt e10s stylo 258.50 -> 235.02
9% damp complicated.netmonitor.open.DAMP linux64 opt e10s stylo 256.25 -> 233.98
8% damp complicated.webconsole.open.DAMP windows10-64-qr opt e10s stylo 357.50 -> 327.14
8% damp complicated.netmonitor.open.DAMP windows10-64 opt e10s stylo 261.90 -> 240.06
8% damp cold.inspector.open.DAMP windows7-32 opt e10s stylo 498.34 -> 456.95
... and many many more similar speedups!

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

So thanks for working on that!

Comment 19

[Jeff Walden [:Waldo]]

Yeah, you get lazy source now -- devtools code in particular, by my recollection of bug 1492937 fixing, was passing explicit "UTF-8" charsets that would have disabled the lazy-source bit. Nice to see things get faster at the same time they're getting simpler!