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

Alexeyzab 3006 speed up store instances #3260

Closed
wants to merge 4 commits into from

Conversation

snoyberg
Copy link
Contributor

This updates #3120 to work with the new extensible snapshots code. Related to #3006. Pinging @alexeyzab.

@snoyberg snoyberg mentioned this pull request Jul 13, 2017
2 tasks
@snoyberg
Copy link
Contributor Author

Possible enhancement: instead of storing a StaticSize 64 ByteString with the base16-encoded value, we could store 4 Int64 values in the unencoded values, which will avoid the ByteString overhead and related heap fragmentation.

@alexeyzab
Copy link
Collaborator

To make sure I understand this correctly, a checklist of what needs to be done here:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.
  • Possibly replace StaticSize 64 ByteString with 4 Int64 values. Would that look something like StaticSize 64 (Int64, Int64, Int64, Int64)?

Should I also remove the commented-out implementations of poke and peek?

@snoyberg
Copy link
Contributor Author

@alexeyzab I'd leave off the Int64 change unless you're particularly enthusiastic about it. If you want to take a crack at it, the best way to define it is:

data StaticSHA256 = StaticSHA256 !Word64 !Word64 !Word64 !Word64

We don't want to use a signed representation since we're just storing the raw bits. And we need the strictness annotations so that GHC can unpack the values directly into the constructor, making the representation much smaller.

@snoyberg
Copy link
Contributor Author

Also, if you're planning on continuing updates on this, can I recommend that you merge my branch into the one you created, and I'll close out this PR?

@alexeyzab
Copy link
Collaborator

@snoyberg Ah, I understand, thanks for explaining that! Sure, is that done by using the Resolve conflicts button on the other PR or is there anything else I can do?

@snoyberg
Copy link
Contributor Author

No, you'll have to do it on your machine. Check out the branch you were working on and run:

git fetch origin
git merge origin/alexeyzab-3006-speed-up-store-instances

Then push to your repo and you should have all the changes.

Depending on how adventurous I'm feeling, I may write a helper library right now for the Word64 trick, unless you tell me you're taking a crack at it.

@alexeyzab
Copy link
Collaborator

Ah, that's what I've done already, wasn't sure if there was something else I missed. Thanks again! Hm, it'd probably take me longer to implement this and I wouldn't want to slow things down. I am curious what that trick would end up looking like though.

@snoyberg
Copy link
Contributor Author

FYI, I don't see the commits I pushed here on your PR, so I don't think it's been pushed to the right branch yet.

@alexeyzab
Copy link
Collaborator

Whoops, my bad. Should be done now.

@snoyberg
Copy link
Contributor Author

Cool, I'll close this up

@snoyberg snoyberg closed this Jul 14, 2017
@snoyberg snoyberg deleted the alexeyzab-3006-speed-up-store-instances branch August 16, 2017 10:55
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