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

[persistent-template] Expose the knot tying logic of parseReferences directly #932

Merged
merged 2 commits into from
Jul 17, 2019

Conversation

merijn
Copy link
Contributor

@merijn merijn commented Jul 17, 2019

TH code generation makes up a significant amount of compile time (see also
#924) and forcing all entities that reference each other to be in a single TH
block means that ALL the code generation has to be redone and recompiled
whenever a single entity changes.

The reason that persistent requires all entities to be in the same TH block is
to properly setup the cross-references from foreign keys when generating a
Migration.

All the logic to build a Migration from a list of EntityDefs is already
exposed in persistent/persistent-template, so it's perfectly possible to
generate the Migration at runtime from independent entity definitions. The
only thing stopping users from doing this is that the logic to setup the
cross-references for foreign keys is...complicated.

parseReferences currently fixes these cross-references for entities in a
single block AND generates the resulting TH. This commit simply moves the logic
for fixing the cross-references into a separate, exported function and has
parseReferences call that function.

The result is that users can now define entities independently (even in
separate modules), use this new function to fix up the cross-references, and
then generate the resulting Migration at runtime.

I've been using this for a while now, defining each entity (and associated
code) in its own module, with no problems and it has substantially reduced the
(re)compile times after schema changes.

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

TH code generation makes up a significant amount of compile time (see also
yesodweb#924) and forcing all entities that reference each other to be in a single TH
block means that ALL the code generation has to be redone and recompiled
whenever a single entity changes.

The reason that persistent requires all entities to be in the same TH block is
to properly setup the cross-references from foreign keys when generating a
Migration.

All the logic to build a Migration from a list of `EntityDef`s is already
exposed in persistent/persistent-template, so it's perfectly possible to
generate the `Migration` at runtime from independent entity definitions. The
only thing stopping users from doing this is that the logic to setup the
cross-references for foreign keys is...complicated.

`parseReferences` currently fixes these cross-references for entities in a
single block AND generates the resulting TH. This commit simply moves the logic
for fixing the cross-references into a separate, exported function and has
`parseReferences` call that function.

The result is that users can now define entities independently (even in
separate modules), use this new function to fix up the cross-references, and
then generate the resulting `Migration` at runtime.

I've been using this for a while now, defining each entity (and associated
code) in its own module, with no problems and it has substantially reduced the
(re)compile times after schema changes.
@parsonsmatt
Copy link
Collaborator

This is fantastic, thanks! Would you be willing to post a gist or short example on how this works to speed up recompile times for you? Not a blocker at all, definitely want to get this in and released :)

@merijn
Copy link
Contributor Author

merijn commented Jul 17, 2019

There's some more code needed to really use it (but with this code exported it can be implemented using just the public API), but I need to extract and polish that code before it can be sensibly added to persistent-template. But I haven't had time to do that yet.

I'd prefer getting this and a new persitent-sqlite version out soon-ish (i.e. before I have time to clean up and add that code). I'm referencing my own code in a paper and I'd like to get a release of my code out that doesn't depend on unreleased git branches on my github to function (hence the sudden push to finish up my outstanding patches ;)).

Once I get done with that work I can definitely expand on how to use this and see if there is a more convenient/higher level API for people.

@parsonsmatt
Copy link
Collaborator

Yeah, I suppose that exposing the building blocks is enough for any potential downstream library or user to implement it. I'll pack a release up today.

@parsonsmatt parsonsmatt merged commit 9965081 into yesodweb:master Jul 17, 2019
@parsonsmatt
Copy link
Collaborator

I've released this and your other patches. Thanks for all the great work!

@merijn
Copy link
Contributor Author

merijn commented Jul 17, 2019

I've got one more incoming, I was working on tests for the RawSqlite stuff, only to find out the behaviour of liftPersist changed between 2.9 and 2.10, so the ported instances compile, but do the wrong thing. Of course, I discover this a whole 5 minutes after you released the new version...sigh

@parsonsmatt
Copy link
Collaborator

Oops 😂 Is the problem in persistent-sqlite or persistent-template? I'll deprecate those versions on Hackage.

@danbroooks
Copy link
Contributor

Thanks for this @merijn :) awesome stuff!

I too would appreciate an example of how to use this, though from what I have seen in the PR this isn't particularly usable at this stage, but a step in that direction?

What else is left to be done? I am really keen to get the compile times down for the Persistent TH in my codebase, as it really is a major bottleneck for us when compiling. If there are other things to be worked on that can get us closer to this goal, I would be happy to take a look 👍

@merijn
Copy link
Contributor Author

merijn commented Sep 13, 2019

@danbroooks Note that it doesn't really reduce the total compile time of the Persistent TH, but it does help reduce incremental compile times, since changes to the schema only need to recompile parts of the TH.

I just got around to messing with the code that uses this and its actually much simpler than I remembered. The only missing bit is pretty trivial (and if I had realised that I'd probably just have it included in this PR.

Basically, I define most entities in their own module, I combine the generated entities into a Migration at runtime and then use this to fix things to it correctly generates all the foreign keys/constraints.

The code to create the migration is pretty trivial, so you could probably easily create a PR to add it (I'm a bit to busy right now, to take care of it).

You can also browse that repo to see how I use it. One of the bonuses of this is that I can actually do incompatible schema migrations pretty easily, since I can keep the old entity TH around in a different module (there's some examples in the V0.hs files under src/Schema/) as "in-between" steps to the final schema.

@merijn merijn deleted the embedEntityDefs branch October 11, 2019 08:31
@merijn merijn restored the embedEntityDefs branch March 16, 2020 09:34
@merijn merijn deleted the embedEntityDefs branch March 16, 2020 09:43
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