Skip to content
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

Add method inlining to inline constructors filter #9599

Merged
merged 30 commits into from Jun 29, 2020

Conversation

basro
Copy link
Contributor

@basro basro commented Jun 16, 2020

Normally inline constructors would immediately cancel if a field that is not a member variable is accessed.

For example:

class MyIterator {
	public var i : Int = 0;
	public inline function new() {}
	public inline function hasNext() return true;
	public inline function next() return i++;
}

// ...

var a = new MyIterator();
a.hasNext(); // The call to hasNext is inlined before the inline constructors runs, no problem here.

var b : Iterator<Int> = a;
b.hasNext(); // Oops, hasNext was not inlined beforehand. 'hasNext' is not a member variable of MyIterator so inlining is cancelled.

With this PR the inline constructor filter will not give up so easily.

When the filter finds an expression of the form ethis.method(args) if ethis references an inline object and method is an inline method in the inline object then it will be inlined on demand by the filter.

This achieves the goals mentioned in #9432

It also probably solves #3147 (requires implementing proper inlineable iterator for map types and fixing the TFor issue I discussed with simn in slack)

I tested using the IntMap iterator from #8806 and this code:

var m = [1 => 2, 3 => 4];
var iter = m.iterator();
while(iter.hasNext()) {
	var i = iter.next();
	trace(i);
}

Compiled into js:

var iter_map_h = { };
iter_map_h[1] = 2;
iter_map_h[3] = 4;
var a = [];
for( var key in iter_map_h ) (iter_map_h.hasOwnProperty(key) ? a.push(key | 0) : null);
var keys = a;
var iter_keys = keys;
var iter_index = 0;
var iter_count = keys.length;
while(iter_index < iter_count) {
	var i = iter_map_h[iter_keys[iter_index++]];
	console.log("src/Main.hx:31:",i);
}

Explanations about some of the changes

Identifying inline objects

Previously inline objects were being identified by the order in which they appeared in the code. While it sounds nice in theory, nested inline objects inside inlined constructors make this very problematic. It requires keeping track of the start and end id of every inlined constructor to be able to reproduce the same id's in the final_pass phase.

In this PR the expressions are now mapped with a function called mark_ctors which will wrap any TNew TArrayDecl or TObjectDecl with @:inlineObject(curr_io_id++). This makes identifying inline objects in final_map a lot simpler, but it's a bit less efficient (Type.map_expr vs Type.iter).

handle_field_case

In analyse_aliases, the function handle_field_case is used to handle analysis of TField and TArray. It used to return either None or Some inline variable that represents a field of an inline object.

handle_field_case now has a third possible return value IOFInlineMethod.

In the cases of TArray and TField this third value is ignored and causes cancellation of the inline object (Keeping the same behaviour it used to have).

IOFInlineMethod is only accepted without cancelling the inline object in the new TCall({eexpr=TField(ethis,fa)},ca) case of analyze_aliases.

@basro
Copy link
Contributor Author

basro commented Jun 17, 2020

I've added a few tests.

tclass *
Type.tparams *
tclass_field *
bool (* Wether the inline constructor is forced or not. Cancelling a forced constructor should produce an error *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using a record type instead of a tuple type here, in my experience that's easier to work with and also easier to modify.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've committed this change.

@skial skial mentioned this pull request Jun 17, 2020
1 task
@basro
Copy link
Contributor Author

basro commented Jun 17, 2020

I found a bug (fix is committed with tests)

The bug should only happen in weird dynamic casts:

class InlineClass {
  public var a : Int;
  public inline function new() {};
  public inline function method() return a;
}

// ...

var a : {function method(arg:String) : Int; } = cast new InlineClass();
// InlineClass.method doesn't take any arguments.
// This should definitely cancel the inlining.
a.method("hello"); 

My fix uses type_iseq_strict to compare a.method type and the InlineClass method cf_type. If false then inlining is cancelled.

This is probably overly conservative, I think that checking if it unifies and that the efield type is not Dynamic would be better. I'm going to try that next.

@RealyUniqueName RealyUniqueName added this to the Backlog milestone Jun 20, 2020
@RealyUniqueName
Copy link
Member

Please update to the latest development

@basro
Copy link
Contributor Author

basro commented Jun 20, 2020

I've rebased on latest development

@Simn Simn merged commit 0d4f2f2 into HaxeFoundation:development Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants