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

tink_template is slow #4

Open
kevinresol opened this issue Jun 22, 2016 · 10 comments
Open

tink_template is slow #4

kevinresol opened this issue Jun 22, 2016 · 10 comments

Comments

@kevinresol
Copy link
Member

Currently tink_template is pretty slow in terms of compile time.
The main problem is that it traverse all members of all types to check for a @:template metadata.
https://github.com/haxetink/tink_template/blob/23aa9f6/src/tink/Template.hx#L107

I think it should better check for some meta at class level first, before diving into all the fields of that class.

@kevinresol
Copy link
Member Author

Adding else return false at line 89 seems to make it faster. But I am not sure about the side effects.

@back2dos
Copy link
Member

It's not hard to change, but it potentially breaks a lot of code that depends on the current behavior. I'm not sure what to do about it.

@kevinresol
Copy link
Member Author

It is pre-1.0.0 anyway...

@back2dos
Copy link
Member

You are right about that. Also the library is not very widely used yet, so better break it sooner than later. Still, I wouldn't want to take it too lightly, particularly because I have another rather big change in mind and I would like to make them simultaneously, to avoid breaking stuff too often.

When it comes to compile time, I think removing the template frontend should yield the most benefits, as the Context.onTypeNotFound scales very poorly, and I'm starting to think the benefit is not worth the cost here. The frontend can still be moved out into another lib.

I think that for field level templates I would prefer for it to be applied if there's a using tink.Template; which can be picked up with Context.getLocalUsing(). People can then use the import.hx to apply it to a whole folder. Quite possibly this might be even a good model for other sugar like tink_lang and tink_async. But I prefer to wait for the final 3.3.0 to depend on its features.

@kevinresol
Copy link
Member Author

Good to know!
Another thing I would like to ask is about compilation-server-friendliness of tink_syntaxhub.
It is purely a question because I don't have a chance to actually use the compilation server yet (thanks minject). But I would like to know if syntaxhub is aware of the compiler cache and skip unnecessary macro re-execution.

@back2dos
Copy link
Member

Yeah, tink_syntaxhub works pretty well with the compilation server. There is one known issue for which I already got a pull request that I'm trying to fully understand myself.

In fact if you do get to use the compiler cache, then the current field level templates are cheap (but the Context.onTypeNotFound is still quite expensive, particularly with deeply nested packages).

If I may go off-topic here: how about moving ufront from minject to dodrugs?

@kevinresol
Copy link
Member Author

Yeah, I would like to do so. But you may already noticed, dodrug doesn't compile on haxe 3.3 at the moment.

@kevinresol
Copy link
Member Author

It turns out that the problem with minject is a simple fix ufront/ufront-mvc#53 (comment)

@jonasmalacofilho
Copy link

jonasmalacofilho commented Aug 10, 2016

@back2dos I'm seeing a lot of time spent on "typing" when using field level templates (a handful of field templates made the build time increase 5 times). I'll looking into this now...

@jonasmalacofilho
Copy link

jonasmalacofilho commented Aug 11, 2016

Just an update: most of the time is being spent on my proposed change to the SyntaxHub (haxetink/tink_syntaxhub#4). Ops! : )

On another front, I implemented something in the lines of "not traversing all class fields" but maintaining backwards compatibility... It didn't seem to make too much difference, but I'll test it some more and it might eventually become a PR.

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

No branches or pull requests

3 participants