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

Association-induced compile time dependencies #1610

Closed
rranelli opened this issue Aug 3, 2016 · 21 comments
Closed

Association-induced compile time dependencies #1610

rranelli opened this issue Aug 3, 2016 · 21 comments

Comments

@rranelli
Copy link
Contributor

@rranelli rranelli commented Aug 3, 2016

Environment

  • Elixir version (elixir -v): 1.3.2
  • Ecto version (mix deps): 1.1.5 & 2.0.3
  • Operating system: debian jessie

Current behavior

I have a project that contains lots of ecto models (over a 100) with lots of relationships between them. I recently noticed that changing one model would trigger the compilation of every other model.

By using mix xref graph I noticed that a given module would have a compile time dependency for every other related model. For example, if I have something like:

## schema_a.ex
defmodule SchemaA do
  use Ecto.Schema
  schema "tableA" do
    belongs_to :b, SchemaB
  end
end

## schema_b.ex
defmodule SchemaB do
  use Ecto.Schema
  schema "tableB" do
    field :lol, :string
    has_many :as, SchemaA
    has_one :C, SchemaC
  end
end

## schema_c.ex
defmodule SchemaC do
  use Ecto.Schema
  schema "tableC" do
    field :lol, :string
    has_many :as, SchemaA
    has_one :C, SchemaC
  end
end

The dependency graph will look like this: (which means that changing any of those files will require recompilation of every other)

screenshot from 2016-08-03 01-11-41

@josevalim was kind enough to help me identify the problem & suggest a workaround using Module.concat/1:

defmodule SchemaC do
  use Ecto.Schema
  schema "tableC" do
    field :lol, :string
    has_many :as, Module.concat(["SchemaA"])
    has_one :C, Module.concat(["SchemaC"])
  end
end

(I recall him mentioning that phoenix's router uses a similar trick with the scope clause)

After I applied the workaround, the number of recompiled files after a change to a particular model changed from 226 to 6. (which drastically reduced the recompilation times & made everyone happy in the office).

Does it make sense for Ecto to provide facilities to avoid such problem? If not, where would be a good place to document this behavior?

@josevalim
Copy link
Member

@josevalim josevalim commented Aug 3, 2016

Yes, I do think we should provide a solution for this given those are not effectively compile time dependencies. One suggestion is to support this syntax:

defmodule MyApp.SchemaC do
  use Ecto.Schema
  schema MyApp, "tableC" do
    field :lol, :string
    has_many :as, SchemaA
    has_one :C, SchemaC
  end
end

It mirrors Phoenix scope and it is explicit about the nesting. @michalmuskala, thoughts?

@michalmuskala
Copy link
Member

@michalmuskala michalmuskala commented Aug 3, 2016

I'm not that familiar with the compiler internals, but why is there a compile-time dependency in the first place? We're not calling any functions on the association module. Is mere presence of the module name somewhere in the code enough to cause the compile-time dependency?

@josevalim
Copy link
Member

@josevalim josevalim commented Aug 3, 2016

Is mere presence of the module name somewhere in the code enough to cause the compile-time dependency?

The presence of a module name in the module body will do it because the compiler doesn't know what Ecto does with it from that moment on. We would need to perform really complex code analysis in order to figure that out correctly.

@rranelli
Copy link
Contributor Author

@rranelli rranelli commented Aug 10, 2016

I've been having a look at Ecto's source and I think I might be able to implement Jose's suggestion.

Should I go ahead and try to do that? Or is it better to wait for more suggestions/discussion?

@josevalim
Copy link
Member

@josevalim josevalim commented Aug 10, 2016

@rranelli please go ahead!

@rranelli
Copy link
Contributor Author

@rranelli rranelli commented Aug 14, 2016

I have already drafted a patch that seems to be working for associations. I am now working on supporting scoped custom types. @josevalim's example did not mention the custom types case, but we do intend to support them, right?

I will probably be done with the code today, but I would like to test the change in my app first to be sure it has the intended effects. This will probably take me a couple of days since we're still using 1.1.5 =(

@josevalim
Copy link
Member

@josevalim josevalim commented Aug 14, 2016

@rranelli hrm, this is though because we don't want Ecto.DateTime (or any other type defined in a library) to be become MyApp.Ecto.DateTime but I understand why one would expect such to happen.

michalmuskala added a commit that referenced this issue Sep 5, 2016
This will prevent the compiler from creating a compile-time dependency
on the association module. It's safe to do, since in that place we
don't call any functions on the association module.

This solves an issue of excessive recompilation in Ecto projects with
heavy use of the associations.

Closes #1610
michalmuskala added a commit that referenced this issue Sep 5, 2016
This will prevent the compiler from creating a compile-time dependency
on the association module. It's safe to do, since in that place we
don't call any functions on the association module.

This solves an issue of excessive recompilation in Ecto projects with
heavy use of the associations.

Closes #1610
josevalim added a commit that referenced this issue Sep 5, 2016
This will prevent the compiler from creating a compile-time dependency
on the association module. It's safe to do, since in that place we
don't call any functions on the association module.

This solves an issue of excessive recompilation in Ecto projects with
heavy use of the associations.

Closes #1610
@atomkirk
Copy link
Contributor

@atomkirk atomkirk commented Oct 11, 2017

So whats the resolution on this? This is causing my project to compile 130 files on most file changes (very association-heavy application) and I'm about to go fix all my associations with Module.concat(ZB, User) in all my schemas. Please stop me if not necessary :)

@josevalim
Copy link
Member

@josevalim josevalim commented Oct 11, 2017

@atomkirk this is supposedly fixed on master. What is your ecto version?

@atomkirk
Copy link
Contributor

@atomkirk atomkirk commented Oct 11, 2017

2.1.6

@josevalim
Copy link
Member

@josevalim josevalim commented Oct 11, 2017

@atomkirk can you provide a sample app taht reproduces the error? because for all purposes this is no supposed to happen. :)

@atomkirk
Copy link
Contributor

@atomkirk atomkirk commented Oct 11, 2017

Well, I haven't a clue why its happening so I doubt I could create a sample app. I suspect this is my own problem to solve and I've accidentally done something wrong. I'm trying to debug it with mix xref and I can't seem to figure out why editing one user.ex file would recompile 136 other files. They are all regular .ex modules in my project. Is there a list somewhere of things that trigger a recompile so I can start to untangle this mess?

@OvermindDL1
Copy link
Contributor

@OvermindDL1 OvermindDL1 commented Oct 11, 2017

Is there a list somewhere of things that trigger a recompile so I can start to untangle this mess?

What about mix xref callers --format pretty --sink MyServer.User or so.

@atomkirk
Copy link
Contributor

@atomkirk atomkirk commented Oct 11, 2017

Ok, yes I've read that article about 3 times in the past month and I'm really struggling to understand why this is happening. I have have two files A -> B and I'm trying to break every possible compile dependency between them and yet changing A recompiles B (and 136 other files). Do you have any suggestions on how I can get help with this without taking this issue further off topic?

@josevalim
Copy link
Member

@josevalim josevalim commented Oct 11, 2017

@atomkirk just to clarify, if A depends on B then changing B will recompile A. In any case, you need to understand why the dependency exists and what is setting it to compile time. Are you sure it is a has_many or could it possibly be something else?

@josevalim
Copy link
Member

@josevalim josevalim commented Oct 11, 2017

You also need to confirm which files are being recompiled, are they really other Ecto schemas? You can use the --verbose flag:

mix compile --verbose
@atomkirk
Copy link
Contributor

@atomkirk atomkirk commented Oct 11, 2017

I replaced all my associations with Module.concat, so I don't think its the ecto associations that are causing the problem. Thats why I'm asking if there is a good way to get help outside of this issue (since my problem does not seem to be the topic of this issue). Maybe I could open a new issue regarding this? I really don't think I'm doing anything crazy with my app. I don't think we'd mind sharing a copy of our app for an example, but probably not posted to a public gh issue.

I'm watching the beam files that change when I recompile with fswatch -r _build/dev | grep 'zipbooks/ebin/.*\.beam$' There are a huge mix of different types of files being recompiled, including most of my schemas. A big hint to my problem is that my main application module is recompiled constantly (which then seems to lead to 100 other modules being recompiled…). If I comment out most of the stuff in my supervision tree, it drops from 136 recompiling to 44.

@atomkirk
Copy link
Contributor

@atomkirk atomkirk commented Oct 11, 2017

Ok I have the go ahead to share a prepared copy of our app with just you if you're interested. It could be something really stupid I did but maybe there's something that could help other elixir beginners from making the same mistake.

@josevalim
Copy link
Member

@josevalim josevalim commented Oct 11, 2017

Please send me a zip (without the .git, _build and deps directory - you most likely can get one from github) to my github e-mail. :)

@atomkirk
Copy link
Contributor

@atomkirk atomkirk commented Oct 11, 2017

Ok, actually, I've figure out whats going on. I did a find and replace in my entire project for in App.Model (like as in queries from i in App.Model, where: …) and replaced them all with Module.concat(App, Model).

Then in my main app module, I did worker/supervisor(Module.concat(App, Module))

And now my app is pretty much fixed. I changed a lot of different files and they all triggered 1-15 other files to be recompiled.

So that some seems very related to this issue. Basically there are a lot of places in the ecto/phoenix/elixir framework that require you to give a macro a module name, which creates a huge compile dependency graph.

Any thoughts on this? I bet you hate the idea of using Module.concat everywhere as much as I do :)

I guess it is still possible this is a problem specific to my project, like maybe I have a bunch of cyclical references and this is the only way to break them, but I don't see an obvious way to fix it.

bartekupartek added a commit to bartekupartek/ecto that referenced this issue Mar 19, 2019
This will prevent the compiler from creating a compile-time dependency
on the association module. It's safe to do, since in that place we
don't call any functions on the association module.

This solves an issue of excessive recompilation in Ecto projects with
heavy use of the associations.

Closes elixir-ecto#1610
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.