New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dart2js: goal directed inlining #9757
Comments
Added this to the Later milestone. |
Added TriageForM5 label. |
Removed TriageForM5 label. |
This really harms generated code readability, I'm spending a decent amount of time just trying to piece together the generated code. In debugging some code, I see code such as element.nodes.firstChild getting inlined 50+ times instance2.get$nodes; In general the generated code appears to be ~2x more lines and much of this appears to be unnecessary. |
I'm suffering in a similar way. |
Issue #15070 has been merged into this issue. |
Set owner to @floitschG. |
Removed this from the 1.6 milestone. |
Removed Oldschool-Milestone-1.6 label. |
Another example of the cost of excessive inlining: Run: tools/test.py -mrelease -cdart2js -rd8 pkg/csslib If inlining is disabled, the size is 804803 |
Another example: final int _duration; const Duration({int days: 0, /** Most call sites use one named argument, like the one inside Duration.+ This is an example where it would be worth generating specializations, doing reducing optimizations on the specializations, and then inlining the specializations when it becomes clear that most of the code is optimized away. It would be pretty challenging to come up with simple heuristics for this without the help of a specializer. Manually forcing inlining gives much better code for Duration arithmetic. |
cc @kmillikin. |
Set owner to @kmillikin. |
Here is another example of low-impact inlining of the JSSyntaxRegExp, in combination with non-sharing of string literals, is quite large t1 = new H.JSSyntaxRegExp("rgb((\d{1,3}),\s*(\d{1,3}),\s*(\d{1,3}))", H.JSSyntaxRegExp_makeNative("rgb((\d{1,3}),\s*(\d{1,3}),\s*(\d{1,3}))", false, true, false), null, null).firstMatch$1(color)._match; Simply not inlining would be better: t1 = H.JSSyntaxRegExp$("rgb((\d{1,3}),\s*(\d{1,3}),\s*(\d{1,3}))", false, true).firstMatch$1(color)._match; |
A heuristic that would work in a subset of the Duration cases is the following:
|
This is probably one of AngularDart's top 3 concerns today. |
Looked at the inlining logic a bit, and it seems like this could be broken down into a few sub-issues:
I'd perhaps be able to take a look at the first two. The third is obviously a bit trickier |
We have stopped development on the Ast based pipeline and are working on the Kernel based pipeline. You are welcome to play with the Ast based pipeline, but be aware that this is now in maintenance mode until we delete it. It is also difficult to work with the Ast for inlining heuristics ( For the Kernel inlining - once everything is working with no inlining we will re-implement inlining. Initially we will implement inlining that is similar to the current inlining (i.e. during SSA construction, but with slightly better heuristics since the inlining candidate is available in a more useful form). We will do it this way because we believe we can get inlining to parity with the Ast pipeline quickly (so we can switch over and delete code). Then we will consider how to reimplement inlining. Deferred inlining will require substantial work on the SSA representation. My experience is that nothing turns out exactly as expected, so please report back any experiments you try. One example is that it appears that one should inline |
dart2js is doing low-impact inlining.
r20909
this._liblib9$_timer =$.Timer_Timer($ .Duration_16000, this._liblib9$_step$0());
r20929
t1 = this._liblib10$_step$0();$.TimerImpl$ (milliseconds, t1);
milliseconds = $.Duration_16000.get$inMilliseconds();
if (milliseconds < 0)
milliseconds = 0;
this._liblib10$_timer =
There is almost no benefit from this inlining - it avoids one call that wraps two others at considerable increase in code size. dart2js should not have bothered with this inlining - the JavaScript engines should be able to do it.
But this inlining could be a lot better if get$inMilliseconds was also inlined. It happens to be a pure function of a field of a constant. It should be possible to do more inlining and reduce the code to
this._liblib10$_timer =$.TimerImpl$ (16, this._liblib10$_step$0());
dart2js should avoid inlining unless it can prove it leads to subsequent optimizations that can't be expected of a good JavaScript engine.
In summary, dart2js should inline when:
The text was updated successfully, but these errors were encountered: