142234 2015-03-03 14:20:00 -0800 JIT debugging features that selectively disable the JITs for code blocks need to stay out of the way of the critical path of JIT management 2015-03-03 14:36:26 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 142229 1 fpizlo fpizlo benjamin mark.lam oldest_to_newest 1073796 0 fpizlo 2015-03-03 14:20:00 -0800 Long ago, we used to selectively disable compilation of CodeBlocks for debugging purposes by adding hacks to DFGDriver.cpp. This was all well and good. It used the existing CompilationFailed mode of the DFG driver to signal failure of CodeBlocks that we didn't want to compile. That's great because CompilationFailed is a well-supported return value on the critical path, usually used for when we run out of JIT memory. Later, this was moved into DFGCapabilities. This was basically incorrect. It introduced a bug where disabling compiling of a CodeBlock meant that we stopped inlining it as well. So if you had a compiler bug that arose if foo was inlined into bar, and you bisected down to bar, then foo would no longer get inlined and you wouldn't see the bug. That's busted. So then we changed the code in DFGCapabilities to mark bar as CanCompile and foo as CanInline. Now, foo wouldn't get compiled alone but it would get inlined. But then we removed CanCompile because that capability mode only existed for the purpose of our old varargs hacks. After that removal, "CanInline" became CannotCompile. This means that if you bisect down on bar in the "foo inlined into bar" case, you'll crash in the DFG because the baseline JIT wouldn't have known to insert profiling on foo. We could fix this by bringing back CanInline. But this is all a pile of nonsense. The debug support to selectively disable compilation of some CodeBlocks shouldn't cross-cut our entire engine and should most certainly never involve adding new capability modes. This support is a hack at best and is for use by JSC hackers only. It should be as unintrusive as possible. So, as in the ancient times, the only proper place to put this hack is in DFGDriver.cpp, and return CompilationFailed. This is correct not just because it takes capability modes out of the picture (and obviates the need to introduce new ones), but also because it means that disabling compilation doesn't change the profiling mode of other CodeBlocks in the Baseline JIT. Capability mode influences profiling mode which in turn influences code generation in the Baseline JIT, sometimes in very significant ways - like, we sometimes do additional double-to-int conversions in Baseline if we know that we might tier-up into the DFG, since this buys us more precise profiling. 1073799 1 247794 fpizlo 2015-03-03 14:23:17 -0800 Created attachment 247794 the patch 1073802 2 247794 mark.lam 2015-03-03 14:26:17 -0800 Comment on attachment 247794 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=247794&action=review r=me > Source/JavaScriptCore/ChangeLog:12 > + at timelike infinity. That makes these hacks much more likely to continue working as we make "timelike"? 1073803 3 247794 mark.lam 2015-03-03 14:27:14 -0800 Comment on attachment 247794 the patch What happened to MacroAssembler::supportsFloatingPoint()? You didn't include that in the new check. 1073804 4 benjamin 2015-03-03 14:28:52 -0800 View in context: https://bugs.webkit.org/attachment.cgi?id=247794&action=review r=me too > Source/JavaScriptCore/ChangeLog:8 > + See bug description for complete analysis. This reduces the intrusiveness of debugging hacks You should copy the description here. We assume that the ChangeLogs will survive bugzilla. 1073805 5 mark.lam 2015-03-03 14:29:28 -0800 (In reply to comment #3) > Comment on attachment 247794 [details] > the patch > > What happened to MacroAssembler::supportsFloatingPoint()? You didn't > include that in the new check. My bad. This didn't need to be moved. 1073806 6 fpizlo 2015-03-03 14:30:35 -0800 (In reply to comment #2) > Comment on attachment 247794 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247794&action=review > > r=me > > > Source/JavaScriptCore/ChangeLog:12 > > + at timelike infinity. That makes these hacks much more likely to continue working as we make > > "timelike"? It's a term of art, see for example http://en.wikipedia.org/wiki/Closed_timelike_curve 1073807 7 fpizlo 2015-03-03 14:30:50 -0800 (In reply to comment #4) > View in context: > https://bugs.webkit.org/attachment.cgi?id=247794&action=review > > r=me too > > > Source/JavaScriptCore/ChangeLog:8 > > + See bug description for complete analysis. This reduces the intrusiveness of debugging hacks > > You should copy the description here. We assume that the ChangeLogs will > survive bugzilla. OK. 1073810 8 fpizlo 2015-03-03 14:36:26 -0800 Landed in http://trac.webkit.org/changeset/180956 247794 2015-03-03 14:23:17 -0800 2015-03-03 14:26:17 -0800 the patch blah.patch text/plain 4899 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTgwOTU1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDMwIEBA CisyMDE1LTAzLTAzICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg SklUIGRlYnVnZ2luZyBmZWF0dXJlcyB0aGF0IHNlbGVjdGl2ZWx5IGRpc2FibGUgdGhlIEpJVHMg Zm9yIGNvZGUgYmxvY2tzIG5lZWQgdG8gc3RheSBvdXQgb2YgdGhlIHdheSBvZiB0aGUgY3JpdGlj YWwgcGF0aCBvZiBKSVQgbWFuYWdlbWVudAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MTQyMjM0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisgICAgICAgIAorICAgICAgICBTZWUgYnVnIGRlc2NyaXB0aW9uIGZvciBjb21wbGV0 ZSBhbmFseXNpcy4gVGhpcyByZWR1Y2VzIHRoZSBpbnRydXNpdmVuZXNzIG9mIGRlYnVnZ2luZyBo YWNrcworICAgICAgICBieSBtYWtpbmcgdGhlbSB1c2UgdGhlIHZlcnkgc2ltcGxlIENvbXBpbGF0 aW9uRmFpbGVkIG1lY2hhbmlzbSByYXRoZXIgdGhhbiB0cnlpbmcgdG8KKyAgICAgICAgaW5mbHVl bmNlIGNhcGFiaWxpdHkgbW9kZXMuIENhcGFiaWxpdHkgbW9kZXMgaGF2ZSB2ZXJ5IHN1YnRsZSBl ZmZlY3RzIG9uIHRoZSB3aG9sZSBlbmdpbmUsCisgICAgICAgIHdoaWxlIENvbXBpbGF0aW9uRmFp bGVkIGp1c3QgbWFrZXMgdGhlIGVuZ2luZSBwcmV0ZW5kIGxpa2UgdGhlIERGRyBjb21waWxhdGlv biB3aWxsIGhhcHBlbgorICAgICAgICBhdCB0aW1lbGlrZSBpbmZpbml0eS4gVGhhdCBtYWtlcyB0 aGVzZSBoYWNrcyBtdWNoIG1vcmUgbGlrZWx5IHRvIGNvbnRpbnVlIHdvcmtpbmcgYXMgd2UgbWFr ZQorICAgICAgICBvdGhlciBjaGFuZ2VzIHRvIHRoZSBzeXN0ZW0uCisgICAgICAgIAorICAgICAg ICBUaGlzIGJyaW5ncyBiYWNrIHRoZSBhYmlsaXR5IHRvIGJpc2VjdCBkb3duIG9udG8gYSBmdW5j dGlvbiBiYXIgd2hlbiBiYXIgaW5saW5lcyBmb28uIFByaW9yCisgICAgICAgIHRvIHRoaXMgY2hh bmdlLCB3ZSB3b3VsZCBjcmFzaCBpbiB0aGF0IGNhc2UuCisKKyAgICAgICAgKiBkZmcvREZHQ2Fw YWJpbGl0aWVzLmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjppc1N1cHBvcnRlZCk6CisgICAgICAg IChKU0M6OkRGRzo6bWlnaHRDb21waWxlRXZhbCk6CisgICAgICAgIChKU0M6OkRGRzo6bWlnaHRD b21waWxlUHJvZ3JhbSk6CisgICAgICAgIChKU0M6OkRGRzo6bWlnaHRDb21waWxlRnVuY3Rpb25G b3JDYWxsKToKKyAgICAgICAgKEpTQzo6REZHOjptaWdodENvbXBpbGVGdW5jdGlvbkZvckNvbnN0 cnVjdCk6CisgICAgICAgICogZGZnL0RGR0NhcGFiaWxpdGllcy5oOgorICAgICAgICAqIGRmZy9E RkdEcml2ZXIuY3BwOgorICAgICAgICAoSlNDOjpERkc6OmNvbXBpbGVJbXBsKToKKwogMjAxNS0w My0wMyAgcGVhdm9Ab3V0bG9vay5jb20gIDxwZWF2b0BvdXRsb29rLmNvbT4KIAogICAgICAgICBb V2luNjRdIEpTQyBjb21waWxlIGVycm9yLgpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL2Rm Zy9ERkdDYXBhYmlsaXRpZXMuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29y ZS9kZmcvREZHQ2FwYWJpbGl0aWVzLmNwcAkocmV2aXNpb24gMTgwOTUwKQorKysgU291cmNlL0ph dmFTY3JpcHRDb3JlL2RmZy9ERkdDYXBhYmlsaXRpZXMuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0z MCwxOSArMzAsMTYgQEAKIAogI2luY2x1ZGUgIkNvZGVCbG9jay5oIgogI2luY2x1ZGUgIkRGR0Nv bW1vbi5oIgotI2luY2x1ZGUgIkRGR0Z1bmN0aW9uV2hpdGVsaXN0LmgiCiAjaW5jbHVkZSAiSW50 ZXJwcmV0ZXIuaCIKICNpbmNsdWRlICJKU0NJbmxpbmVzLmgiCiAjaW5jbHVkZSAiT3B0aW9ucy5o IgogCiBuYW1lc3BhY2UgSlNDIHsgbmFtZXNwYWNlIERGRyB7CiAKLWJvb2wgaXNTdXBwb3J0ZWQo Q29kZUJsb2NrKiBjb2RlQmxvY2spCitib29sIGlzU3VwcG9ydGVkKCkKIHsKICAgICByZXR1cm4g T3B0aW9uczo6dXNlREZHSklUKCkKLSAgICAgICAgJiYgTWFjcm9Bc3NlbWJsZXI6OnN1cHBvcnRz RmxvYXRpbmdQb2ludCgpCi0gICAgICAgICYmIE9wdGlvbnM6OmJ5dGVjb2RlUmFuZ2VUb0RGR0Nv bXBpbGUoKS5pc0luUmFuZ2UoY29kZUJsb2NrLT5pbnN0cnVjdGlvbkNvdW50KCkpCi0gICAgICAg ICYmIEZ1bmN0aW9uV2hpdGVsaXN0OjplbnN1cmVHbG9iYWxXaGl0ZWxpc3QoKS5jb250YWlucyhj b2RlQmxvY2spOworICAgICAgICAmJiBNYWNyb0Fzc2VtYmxlcjo6c3VwcG9ydHNGbG9hdGluZ1Bv aW50KCk7CiB9CiAKIGJvb2wgaXNTdXBwb3J0ZWRGb3JJbmxpbmluZyhDb2RlQmxvY2sqIGNvZGVC bG9jaykKQEAgLTUzLDIyICs1MCwyMiBAQCBib29sIGlzU3VwcG9ydGVkRm9ySW5saW5pbmcoQ29k ZUJsb2NrKiBjCiAKIGJvb2wgbWlnaHRDb21waWxlRXZhbChDb2RlQmxvY2sqIGNvZGVCbG9jaykK IHsKLSAgICByZXR1cm4gaXNTdXBwb3J0ZWQoY29kZUJsb2NrKQorICAgIHJldHVybiBpc1N1cHBv cnRlZCgpCiAgICAgICAgICYmIGNvZGVCbG9jay0+aW5zdHJ1Y3Rpb25Db3VudCgpIDw9IE9wdGlv bnM6Om1heGltdW1PcHRpbWl6YXRpb25DYW5kaWRhdGVJbnN0cnVjdGlvbkNvdW50KCk7CiB9CiBi b29sIG1pZ2h0Q29tcGlsZVByb2dyYW0oQ29kZUJsb2NrKiBjb2RlQmxvY2spCiB7Ci0gICAgcmV0 dXJuIGlzU3VwcG9ydGVkKGNvZGVCbG9jaykKKyAgICByZXR1cm4gaXNTdXBwb3J0ZWQoKQogICAg ICAgICAmJiBjb2RlQmxvY2stPmluc3RydWN0aW9uQ291bnQoKSA8PSBPcHRpb25zOjptYXhpbXVt T3B0aW1pemF0aW9uQ2FuZGlkYXRlSW5zdHJ1Y3Rpb25Db3VudCgpOwogfQogYm9vbCBtaWdodENv bXBpbGVGdW5jdGlvbkZvckNhbGwoQ29kZUJsb2NrKiBjb2RlQmxvY2spCiB7Ci0gICAgcmV0dXJu IGlzU3VwcG9ydGVkKGNvZGVCbG9jaykKKyAgICByZXR1cm4gaXNTdXBwb3J0ZWQoKQogICAgICAg ICAmJiBjb2RlQmxvY2stPmluc3RydWN0aW9uQ291bnQoKSA8PSBPcHRpb25zOjptYXhpbXVtT3B0 aW1pemF0aW9uQ2FuZGlkYXRlSW5zdHJ1Y3Rpb25Db3VudCgpOwogfQogYm9vbCBtaWdodENvbXBp bGVGdW5jdGlvbkZvckNvbnN0cnVjdChDb2RlQmxvY2sqIGNvZGVCbG9jaykKIHsKLSAgICByZXR1 cm4gaXNTdXBwb3J0ZWQoY29kZUJsb2NrKQorICAgIHJldHVybiBpc1N1cHBvcnRlZCgpCiAgICAg ICAgICYmIGNvZGVCbG9jay0+aW5zdHJ1Y3Rpb25Db3VudCgpIDw9IE9wdGlvbnM6Om1heGltdW1P cHRpbWl6YXRpb25DYW5kaWRhdGVJbnN0cnVjdGlvbkNvdW50KCk7CiB9CiAKSW5kZXg6IFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHQ2FwYWJpbGl0aWVzLmgKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdDYXBhYmlsaXRpZXMuaAkocmV2aXNpb24gMTgwOTUw KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdDYXBhYmlsaXRpZXMuaAkod29ya2lu ZyBjb3B5KQpAQCAtMzgsNyArMzgsNyBAQCBuYW1lc3BhY2UgSlNDIHsgbmFtZXNwYWNlIERGRyB7 CiAjaWYgRU5BQkxFKERGR19KSVQpCiAvLyBGYXN0IGNoZWNrIGZ1bmN0aW9uczsgaWYgdGhleSBy ZXR1cm4gdHJ1ZSBpdCBpcyBzdGlsbCBuZWNlc3NhcnkgdG8KIC8vIGNoZWNrIG9wY29kZXMuCi1i b29sIGlzU3VwcG9ydGVkKENvZGVCbG9jayopOworYm9vbCBpc1N1cHBvcnRlZCgpOwogYm9vbCBp c1N1cHBvcnRlZEZvcklubGluaW5nKENvZGVCbG9jayopOwogYm9vbCBtaWdodENvbXBpbGVFdmFs KENvZGVCbG9jayopOwogYm9vbCBtaWdodENvbXBpbGVQcm9ncmFtKENvZGVCbG9jayopOwpJbmRl eDogU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdEcml2ZXIuY3BwCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZHRHJpdmVyLmNwcAkocmV2aXNpb24gMTgwOTUw KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdEcml2ZXIuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0zMCw2ICszMCw3IEBACiAjaW5jbHVkZSAiSlNTdHJpbmcuaCIKIAogI2luY2x1ZGUg IkNvZGVCbG9jay5oIgorI2luY2x1ZGUgIkRGR0Z1bmN0aW9uV2hpdGVsaXN0LmgiCiAjaW5jbHVk ZSAiREZHSklUQ29kZS5oIgogI2luY2x1ZGUgIkRGR1BsYW4uaCIKICNpbmNsdWRlICJERkdUaHVu a3MuaCIKQEAgLTYyLDYgKzYzLDEwIEBAIHN0YXRpYyBDb21waWxhdGlvblJlc3VsdCBjb21waWxl SW1wbCgKIHsKICAgICBTYW1wbGluZ1JlZ2lvbiBzYW1wbGluZ1JlZ2lvbigiREZHIENvbXBpbGF0 aW9uIChEcml2ZXIpIik7CiAgICAgCisgICAgaWYgKCFPcHRpb25zOjpieXRlY29kZVJhbmdlVG9E RkdDb21waWxlKCkuaXNJblJhbmdlKGNvZGVCbG9jay0+aW5zdHJ1Y3Rpb25Db3VudCgpKQorICAg ICAgICB8fCAhRnVuY3Rpb25XaGl0ZWxpc3Q6OmVuc3VyZUdsb2JhbFdoaXRlbGlzdCgpLmNvbnRh aW5zKGNvZGVCbG9jaykpCisgICAgICAgIHJldHVybiBDb21waWxhdGlvbkZhaWxlZDsKKyAgICAK ICAgICBudW1Db21waWxhdGlvbnMrKzsKICAgICAKICAgICBBU1NFUlQoY29kZUJsb2NrKTsK