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

Add bytesRead to Data.Serialize.Get #62

Merged
merged 2 commits into from
Nov 27, 2016
Merged

Conversation

Ongy
Copy link
Contributor

@Ongy Ongy commented Oct 22, 2016

After another 2 years #8 gets a PR.

I added the current byte count to the state of Get.
Running the benchmark locally, I can't see a major performance hit (only ran each version once though).

I'm a bit unsure about the MonadPlus instance and how to add tests. Can you please have a look at those?

Also #8 also mentions the idea of a bytesWritten. Looking at the current Data.Serialize.Put I don't think it is possible. Was the rewrite already done? Are there plans to go to something similar to Data.Serialize.Get?

@Ongy
Copy link
Contributor Author

Ongy commented Oct 22, 2016

So I played around with bytesWritten for Data.Serialize.Put and I don't think it's feasable without a noticable performance loss.

Even putting in seq to force order got me 10% longer times in the benchmarks.

@elliottt
Copy link
Contributor

I think that the mplus implementation makes sense -- you don't need a w parameter in the try function, as you want to reuse the original w0 value passed in.

Have you been able to benchmark this to see if it causes much performance loss in Get?

@Ongy
Copy link
Contributor Author

Ongy commented Oct 25, 2016

I have tested this on 2 machines. My laptop (i3-2350M) and my pc (i7-2700k) with the benchmark in the repo:
Laptop-head Laptop-PR
PC-head PC-PR

I don't have any other code I can use to do any serious testing.

@elliottt
Copy link
Contributor

It looks like put is going to build a big thunk, could you make the length parameter to it strict?

@elliottt
Copy link
Contributor

Looks like I dropped the ball a little bit on this, thanks for the contribution!

@elliottt elliottt merged commit d5a9957 into GaloisInc:master Nov 27, 2016
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

2 participants