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

Fix #778 | persistent-template - Add persistManyFileWith #791

Merged
merged 4 commits into from
Mar 8, 2018

Conversation

naushadh
Copy link
Contributor

@naushadh naushadh commented Feb 25, 2018

Address persistent#778.

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
    • Linked to Issue instead since it has reference to the why and usage sample for new API
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

qRunIO $ SIO.hSetEncoding h SIO.utf8_bom
s <- qRunIO $ TIO.hGetContents h
ss <- mapM getS fps
let s = mconcat ss
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noticed a minor hiccup when playing with this. Should any of the Entity files be missing a line-break at the EOF, then we end up gluing them wrong. Ex:

  • Foo/Entity
Foo
  name Text
  age Natural
  • Bar/Entity
Bar
  title Text

  • currently will be read as:
Foo
  name Text
  age NaturalBar
  title Text

Should this be updated to T.intercalate "\n" ss to be fault tolerant? Or should we leave it as is and just document that all files must end with a line break (since it is a common convention anyway)?

@MaxGabriel
Copy link
Member

I’m strongly in favor of the fault tolerant approach—it’s great when everything just works!

@MaxGabriel
Copy link
Member

@naushadh You don't have to, but any chance you can confirm that this solves memory usage issues? It'd be nice to point to a solution for the people with 30GB compiles.

@naushadh
Copy link
Contributor Author

@MaxGabriel I tried to reproduce it with my sample project naushadh/persistent-examples/model-organization with 20 entities, but I see ~160MB of memory usage for all cases:

  • monolith (no .stack-work)
  • monolith with a Model change
  • component style (no .stack-work)
  • component style with a Model change

Do you have a sample repo/gist/snippet handy that can repro the multi GB compile issue? I can try breaking it apart component style to see if it makes it go away.

@MaxGabriel
Copy link
Member

@naushadh So I created ~30 models, each deriving Show, Eq, and Generic in this repo: https://github.com/MaxGabriel/persistence-of-memory

I tried using both a single model file and split across 3 model files, but the compile time and memory usage are pretty similar for me (~85 seconds, ~950 MB to compile). I'm compiling with stack build --ghc-options='-j +RTS -s -RTS'. Am I missing something there?

@naushadh
Copy link
Contributor Author

naushadh commented Mar 2, 2018

@MaxGabriel the repo didn't actually utilize the new API persistManyFileWith for migration. I've pushed a PR that shows how to utilize the API along with consuming the smaller model files from separate modules (which GHC can then build in parallel).

Stats

samples memory (MB) time (s)
baseline 877 49.385
new 824 25.079
new+tricks* 1009 21.373

Tricks (since we aren't actually saving much memory)

  • last sample shows cut down GC time by throwing more memory at it
  • hyper-threading can be exploited by invoking -N8 (on a 4 core machine) for GHC 8.2+. However the current example does not have enough independent models/modules to demo its benefits.

Edit: fixed typos.

@MaxGabriel
Copy link
Member

Ah ok great. I had used persistManyFileWith but I didn't realize I needed to add separate modules and run mkPersist in each of them.

I made a PR to your branch adding an example to the documentation: naushadh#3

Otherwise I think this is ready to merge. One final thought: How do you feel about the name? I'm constantly having to look up persistManyFileWith because it's kind of an awkward, but I guess it matches existing conventions.

@naushadh
Copy link
Contributor Author

naushadh commented Mar 3, 2018

Docs look beautiful! Much easier to consume than following 3 levels of urls.

Yeah I thought the same, so I went with convention. I'm open to suggestions.

@MaxGabriel MaxGabriel merged commit 91e474f into yesodweb:master Mar 8, 2018
@MaxGabriel
Copy link
Member

Ok, released on hackage! Thanks for your work on this, it'll be great to point Persistent users to this solution

@naushadh naushadh deleted the fix-778 branch December 29, 2022 00:11
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.

2 participants