Closed Bug 1355155 Opened 5 years ago Closed 4 years ago
Scalar replacement for arrow function
I implemented scalar replacement for MLambdaArrow based on MLambda. This about halves the runtime of six-speed-arrow-declare-es6. We don't actually seem to remove the MNewCallObject instruction, so removing that allocation should a lot. I am not quite sure why this isn't working, maybe because MLambdaArrow is only removed during the sink phase?
We seem to fail the readWithDefault in jit::InlineFrameIterator::findNextFrame with this. I wonder if this something that could happen with normal lambdas as well and this just wasn't observed by tests? I wonder what the difference is. Probably we need to do something special about newTargets, but everything else should be similar. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d058c339d186090d1c4f02082d8c883fd92f7246&selectedJob=90463042
Nicolas I hope you can take a look at this after your are back.
(In reply to Tom Schuster [:evilpie] from comment #1) > We seem to fail the readWithDefault in jit::InlineFrameIterator::findNextFrame with this. […] The problem you are seeing is due to the fact that when we are inlining lambda, we still need a JSFunction to know the number of formal arguments, and the number of locals of the function. Scalar replacement goal is to get rid of this function allocation, thus we cannot use this function for iterating the stack, which is why we have a new RValueAllocation, which provides a default value. The default value is the template object of the function, which is provided when encoding the snapshot . The template object is basically used for reading properties of the JSFunction, but in some cases, such as `f.caller` we have to reconstruct a unique JSFunction corresponding to the lambda, and not return the template object. In such cases, we have to bailout ahead-of-time (recovering values of a frame which is higher on the stack), and resume the bailout once we resume the execution of the frame with the inlined lambda. You can find all the modification made to support MLambda in Scalar Replacement in Bug 1073033.  http://searchfox.org/mozilla-central/source/js/src/jit/shared/CodeGenerator-shared.cpp#413-421 (In reply to Tom Schuster [:evilpie] from comment #0) > […], maybe because MLambdaArrow is only removed during the sink phase? All allocations are removed during the Sink phase, this is true for MNewObject and others. The current implementation of Scalar Replacement only deals with the recovery of the fields/properties of the objects/array.
Thanks Nicolas! After handling LambdaArrow in the Codegenerator-shared code we seem to not have any more test failures. https://treeherder.mozilla.org/#/jobs?repo=try&revision=291fec4bec90de66d4cc1bfe95bf78cc2c4ddad0&selectedJob=92508848
Comment on attachment 8859537 [details] [diff] [review] v2 - Scalar replacement for arrow function Review of attachment 8859537 [details] [diff] [review]: ----------------------------------------------------------------- Add test case similar to jit-test/tests/ion/recover-lambdas*.js , and a micro benchmark similar to misc-forEach-custom, to check sanity and performance boost. And even better if you can land the benchmark before the optimization :)  https://github.com/h4writer/arewefastyet/blob/master/benchmarks/misc/tests/assorted/misc-forEach-custom.js
Attachment #8859537 - Flags: review?(nicolas.b.pierron) → review+
We have this benchmark: https://github.com/h4writer/arewefastyet/blob/master/benchmarks/six-speed/tests/arrow-declare.es6 I am actually having some trouble getting any tests to work, for example recover-lambda.js doesn't seem to work either. IONFLAGS=escape doesn't show anything. I did verify that lambda_arrow is eliminated on the six-speed benchmark, but we still have an unnecessary newcallobject instruction.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/98dec674f3c9 Scalar replacement for arrow functions. r=nbp
So, just landed this without tests ... We do use arrow functions often enough that this optimization triggers, but actually writing a test-case has been difficult. I will keep this bug open and hope nbp can help when he comes back.
At least I can confirm that this is working: https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=arrow-declare-es6
Can you maybe write a short test for this?
Doesn't seem like I am going to get a reply. So far no fuzz bugs!
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
You need to log in before you can comment on or make changes to this bug.