Skip to content
This repository has been archived by the owner on Aug 12, 2020. It is now read-only.

Update #190

Merged
merged 5 commits into from
Nov 7, 2017
Merged

Update #190

merged 5 commits into from
Nov 7, 2017

Conversation

daviddias
Copy link
Contributor

Finding some issues while updating the deps:

  • fix missing code in js-multihash
  • update test to not assume that we implement all the hash functions
  • Figure out why there are tests still failing

@ghost ghost assigned daviddias Oct 20, 2017
@ghost ghost added the status/in-progress In progress label Oct 20, 2017
@daviddias
Copy link
Contributor Author

daviddias commented Oct 20, 2017

@Beanow I followed #187 (comment) and started updating the deps.

I'm being unsuccessful getting unixfs-engine tests to pass with pull-block 1.2.1. With 1.2.0 they pass just fine. Did you have the chance to try running them in your machine?

//cc @dignifiedquire @pgte

@Beanow
Copy link

Beanow commented Oct 21, 2017

@diasdavid nice find. It's looking like a performance regression for a test-case that I hadn't considered in previous tests. I was able to reproduce and quantify this with the other benchmarking tools I've set up.

In chromium:

Test Version Min AVG Max
perByteSmall v1.2.1 4ms 5ms 21ms
perByteSmall v1.2.0 1ms 2ms 4ms
perByteMedium v1.2.1 67ms 69ms 127ms
perByteMedium v1.2.0 23ms 24ms 25ms

Essentially in test/test-fixed-size-chunker.js
The testing method used is

pull(
  pull.values(rawFile),
  chunker(KiB256),
  //...
)

This usage of pull.values(rawFile) feeds the chunker one byte at a time as opposed to more sizable buffers. This will cause it to do 262144 receptions of a single byte buffer and 262144 merges of those buffers per block. Which contains more logic in the later version.

I'll have a look at this scenario and you can look forward to pull-block 1.2.2 soon 🤓

Note that the tests fail due to an 80 second timeout. So the output is still correct, just very slow with these particular tests. I'll let you decide if you want to tweak parameters and release 1.2.1 or wait for a fix.

@dignifiedquire
Copy link
Contributor

dignifiedquire commented Oct 21, 2017 via email

@daviddias daviddias removed their assignment Oct 22, 2017
@ghost ghost assigned daviddias Nov 7, 2017
@daviddias
Copy link
Contributor Author

I'm locking pull-block to 1.2.0 to unblock other updates. Once pull-block 1.2.2 is finished, I'll updated it right away :)

@daviddias daviddias merged commit d88a561 into master Nov 7, 2017
@daviddias daviddias deleted the update branch November 7, 2017 09:05
@ghost ghost removed the status/in-progress In progress label Nov 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants