Bug 174818

Created attachment 316373 [details] patch

Attachment 316373 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3LowerToAir.cpp:1022: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:278: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 316373 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=316373&action=review > Source/JavaScriptCore/ChangeLog:10 > + 1. Don't tier up small functions without loops. We shouldn't do these kinds of heuristics. BBQ is not meant to generate good code even for small functions. This could be leaving too much money on the table.

(In reply to Filip Pizlo from comment #3) > Comment on attachment 316373 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316373&action=review > > > Source/JavaScriptCore/ChangeLog:10 > > + 1. Don't tier up small functions without loops. > > We shouldn't do these kinds of heuristics. BBQ is not meant to generate > good code even for small functions. This could be leaving too much money on > the table. I initially decided to auto-OMG instead of never tiering, and that provided little size savings while costing in compile time (I expected a bump, but it was quite large). The no-tier approach has similar size savings, and WasmBench+TitzerBench are neutral / slightly better. So I agree that BBQ isn't trying to be good, but from the code we're currently seeing it seems like we just have a bunch of small functions which almost never execute, and which definitely aren't hot. I think the i-cache cost alone when we do execute them costs more than whatever non-loop optimization we're giving up (the code size goes down, and from inspection we're not losing much compared to OMG).

(In reply to JF Bastien from comment #4) > (In reply to Filip Pizlo from comment #3) > > Comment on attachment 316373 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=316373&action=review > > > > > Source/JavaScriptCore/ChangeLog:10 > > > + 1. Don't tier up small functions without loops. > > > > We shouldn't do these kinds of heuristics. BBQ is not meant to generate > > good code even for small functions. This could be leaving too much money on > > the table. > > I initially decided to auto-OMG instead of never tiering, and that provided > little size savings while costing in compile time (I expected a bump, but it > was quite large). The no-tier approach has similar size savings, and > WasmBench+TitzerBench are neutral / slightly better. > > So I agree that BBQ isn't trying to be good, but from the code we're > currently seeing it seems like we just have a bunch of small functions which > almost never execute, and which definitely aren't hot. I think the i-cache > cost alone when we do execute them costs more than whatever non-loop > optimization we're giving up (the code size goes down, and from inspection > we're not losing much compared to OMG). The cost of your heuristic is that small functions that do rely on optimization could fall off of a cliff, and your heuristic has no way of detecting when this could happen. I think you should remove this heuristic from your patch, even if this brings back some bloat.

(In reply to Filip Pizlo from comment #5) > (In reply to JF Bastien from comment #4) > > (In reply to Filip Pizlo from comment #3) > > > Comment on attachment 316373 [details] > > > patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=316373&action=review > > > > > > > Source/JavaScriptCore/ChangeLog:10 > > > > + 1. Don't tier up small functions without loops. > > > > > > We shouldn't do these kinds of heuristics. BBQ is not meant to generate > > > good code even for small functions. This could be leaving too much money on > > > the table. > > > > I initially decided to auto-OMG instead of never tiering, and that provided > > little size savings while costing in compile time (I expected a bump, but it > > was quite large). The no-tier approach has similar size savings, and > > WasmBench+TitzerBench are neutral / slightly better. > > > > So I agree that BBQ isn't trying to be good, but from the code we're > > currently seeing it seems like we just have a bunch of small functions which > > almost never execute, and which definitely aren't hot. I think the i-cache > > cost alone when we do execute them costs more than whatever non-loop > > optimization we're giving up (the code size goes down, and from inspection > > we're not losing much compared to OMG). > > The cost of your heuristic is that small functions that do rely on > optimization could fall off of a cliff, and your heuristic has no way of > detecting when this could happen. > > I think you should remove this heuristic from your patch, even if this > brings back some bloat. Tiering is the biggest contributor to bloat on small functions: it represents over half of a small function's size. The other things help push some functions a JIT granule lower (at this point shaving two more instructions would get many of them them one more granule lower, hence #174819). Do you think the patch is worth it without this tiering heuristic? On small functions without loops I have a hard time believing that BBQ can generate egregiously bad code: the CPU can eat up a bunch of straight-line ILP even if slightly sillier. My measurements show that at least for our benchmarks it doesn't matter (I ran them overnight on x86-64 and ARM64), and for these small straight-line code functions it is a code reduction, removing code that was on the critical path. So I can remove 1., but really that's the meat of the change. Reconsider?

(In reply to JF Bastien from comment #6) > (In reply to Filip Pizlo from comment #5) > > (In reply to JF Bastien from comment #4) > > > (In reply to Filip Pizlo from comment #3) > > > > Comment on attachment 316373 [details] > > > > patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=316373&action=review > > > > > > > > > Source/JavaScriptCore/ChangeLog:10 > > > > > + 1. Don't tier up small functions without loops. > > > > > > > > We shouldn't do these kinds of heuristics. BBQ is not meant to generate > > > > good code even for small functions. This could be leaving too much money on > > > > the table. > > > > > > I initially decided to auto-OMG instead of never tiering, and that provided > > > little size savings while costing in compile time (I expected a bump, but it > > > was quite large). The no-tier approach has similar size savings, and > > > WasmBench+TitzerBench are neutral / slightly better. > > > > > > So I agree that BBQ isn't trying to be good, but from the code we're > > > currently seeing it seems like we just have a bunch of small functions which > > > almost never execute, and which definitely aren't hot. I think the i-cache > > > cost alone when we do execute them costs more than whatever non-loop > > > optimization we're giving up (the code size goes down, and from inspection > > > we're not losing much compared to OMG). > > > > The cost of your heuristic is that small functions that do rely on > > optimization could fall off of a cliff, and your heuristic has no way of > > detecting when this could happen. > > > > I think you should remove this heuristic from your patch, even if this > > brings back some bloat. > > Tiering is the biggest contributor to bloat on small functions: it > represents over half of a small function's size. The other things help push > some functions a JIT granule lower (at this point shaving two more > instructions would get many of them them one more granule lower, hence > #174819). > > Do you think the patch is worth it without this tiering heuristic? On small > functions without loops I have a hard time believing that BBQ can generate > egregiously bad code: the CPU can eat up a bunch of straight-line ILP even > if slightly sillier. My measurements show that at least for our benchmarks > it doesn't matter (I ran them overnight on x86-64 and ARM64), and for these > small straight-line code functions it is a code reduction, removing code > that was on the critical path. > > So I can remove 1., but really that's the meat of the change. Reconsider? I think it's still worth landing the change even without 1. I think we should figure out how to make the tiering checks less expensive somehow. I don't think it's OK to say that some functions never tier. That's just too dangerous and breaks too many of our assumptions.

Created attachment 316386 [details] patch > I think it's still worth landing the change even without 1. I think we > should figure out how to make the tiering checks less expensive somehow. Sure, here's an updated patch. I agree we want to make it cheaper. An interpreter would be a good fix, or an intermediate helper on all non-tiered function entry for tier-up check (which would handle non-loop recording and tier-up). I agree with either of these the no-tierup approach isn't as useful. > I don't think it's OK to say that some functions never tier. That's just > too dangerous and breaks too many of our assumptions. Eh, I disagree but it's not worth arguing. So much of the code is small and trivial, straight-line. If BBQ messes it up worst than a few extra registers and spills then we should fix that bug. I'd have liked to have a small gain, but we can get back to it in a few months.

Attachment 316386 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3LowerToAir.cpp:1022: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

*** Bug 169792 has been marked as a duplicate of this bug. ***

Comment on attachment 316386 [details] patch Clearing flags on attachment: 316386 Committed r219899: <http://trac.webkit.org/changeset/219899>

All reviewed patches have been landed. Closing bug.