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

Split big modules into smaller pieces #58

Merged
merged 98 commits into from
Dec 21, 2017
Merged

Split big modules into smaller pieces #58

merged 98 commits into from
Dec 21, 2017

Conversation

tfausak
Copy link
Owner

@tfausak tfausak commented Dec 14, 2017

Right now each module in Rattletrap includes a type, a way to encode that type as bytes/bits, and a way to decode that type from bytes/bits. This pull request splits those modules apart into one each for types, encoding, and decoding. It's still a work in progress. There are a lot of modules! By doing this, I hope to:

  • Speed up iterative development. With things split up like this, a lot of recompilation can be avoided. In particular, the types will almost never change. That means I shouldn't have to pay to recompile them.

  • Break up the gigantic JSON module by pushing the derivations closer to the types. This will also speed up iterative development a lot.

  • Spot and remove commonalities between all types, encoders, and decoders. I will probably end up with a module like Rattletrap.Encode.Common that drags in all the typical stuff. That should avoid a bunch of repetitive imports.

Doing this does have one big downside: It flattens the module hierarchy. Rattletrap has a lot of types. I don't know exactly, but there will probably be about 50-100 Rattletrap.Type.* modules. That can be kind of annoying to sift through. On the other hand, it's pretty easy for most editors to quickly find what you're looking for.

@tfausak tfausak self-assigned this Dec 14, 2017
@tfausak
Copy link
Owner Author

tfausak commented Dec 14, 2017

Oh, another benefit of doing things this way is that it makes splitting things up into packages easier. There's no reason to do this right now, but it's possible I could end up with rattletrap-types, rattletrap-decoding, and so on.

@tfausak
Copy link
Owner Author

tfausak commented Dec 15, 2017

The scope of this PR has ... crept.

@tfausak
Copy link
Owner Author

tfausak commented Dec 17, 2017

Now that I'm back in the world of laziness, I should make sure that I don't have any space leaks: https://neilmitchell.blogspot.com/2015/09/detecting-space-leaks.html

@tfausak
Copy link
Owner Author

tfausak commented Dec 17, 2017

At this point I'm pretty happy with where the code is at. There are still some things that would be nice to clean up, but overall it's a lot better than it used to be. One problem is that now it's a lot slower: https://gist.github.com/tfausak/83a8883f68fa87450f94bca83be07ff9

I haven't been paying attention to performance at all. It's possible there is an easy fix here. I'm not too concerned at this point.

@tfausak
Copy link
Owner Author

tfausak commented Dec 18, 2017

I should make sure that I don't have any space leaks

I didn't find any space leaks.

might let me get rid of reverseBytes

Nope, not going to happen. Each byte is backwards, and then the bits in each thing are backwards again.

Investigate moving test replays to Git LFS.

Not worth it. The replays are already in the Git history. Also now that the test suite can handle replays not existing, I could just point to replays on S3 or something.

Here's a list of what's left to do:

  • Profile some runs to make sure there aren't any obvious hot spots.
  • Investigate using "blocks" for bitwise getters to improve performance.
  • Decode chunks of frames independently. This can probably go in a separate PR.
  • Clean up all the encoders in the same way that I did for the decoders.
  • Push more stuff into the common modules to avoid more imports.
  • Maybe switch to ReaderT instead of passing around a bunch of arguments.
  • See if compressed words can be cleaned up or sped up.

@tfausak
Copy link
Owner Author

tfausak commented Dec 20, 2017

Alright, I think the scope has crept enough here. I still have a wishlist, but the big stuff has been done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant