Minor subscript loader followup fixes after bug 1492937
Categories
(Core :: XPConnect, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
No description provided.
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.
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.
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.
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/
Updated•6 years ago
|
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...
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?
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
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.
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...
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.
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.
Updated•6 years ago
|
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
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
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
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.
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 18•6 years ago
|
||
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!
Assignee | ||
Comment 19•6 years ago
|
||
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!
Description
•