Jeff Walden [:Waldo] Assignee
	Description • 6 years ago

      No description provided.

	Jeff Walden [:Waldo] Assignee
	Comment 3 • 6 years ago

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.

	Kris Maglione [:kmag]
	Comment 4 • 6 years ago

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.

	Kris Maglione [:kmag]
	Comment 5 • 6 years ago

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.

	Jan de Mooij [:jandem]
	Comment 6 • 6 years ago

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

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

\o/

	Jeff Walden [:Waldo] Assignee
	Comment 7 • 6 years ago

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

	Kris Maglione [:kmag]
	Comment 8 • 6 years ago

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

	Kris Maglione [:kmag]
	Comment 9 • 6 years ago

(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

	Jeff Walden [:Waldo] Assignee
	Comment 10 • 6 years ago

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.

	Jeff Walden [:Waldo] Assignee
	Comment 11 • 6 years ago

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

	Jeff Walden [:Waldo] Assignee
	Comment 12 • 6 years ago

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.

	Kris Maglione [:kmag]
	Comment 13 • 6 years ago

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

	Pulsebot
	Comment 14 • 6 years ago

Pushed by jwalden@mit.edu:
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

	Eliza Balazs [:ebalazs_]
	Comment 15 • 6 years ago
bugherder

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

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 16 • 6 years ago

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

	Jeff Walden [:Waldo] Assignee
	Comment 17 • 6 years ago

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