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

blocks/blockstore: Get ARC cache and context passing #2942

Closed
wants to merge 8 commits into from

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Jul 5, 2016

No description provided.

@Kubuxu Kubuxu added the status/in-progress In progress label Jul 5, 2016
@Kubuxu Kubuxu force-pushed the feature/blocks-cache-arc branch 2 times, most recently from f27106c to 4286953 Compare July 5, 2016 15:24
@Kubuxu Kubuxu added need/review Needs a review and removed status/in-progress In progress labels Jul 5, 2016
@Kubuxu Kubuxu added this to the ipfs-0.4.3 milestone Jul 5, 2016
@whyrusleeping
Copy link
Member

@Kubuxu this PR results in a small decrease in performance on the 'DirAddOpsPerSec' benchmark in ipfs-whatever:

Results  Before  After  % Change
PatchOpsPerSec   71.43   85.76   20.06%
DirAddOpsPerSec  528.10  491.64  -6.90%
Add10MBTime      443.56  439.88  -0.83%
Add10MBStdev     20.63   17.23   -16.48%

@Kubuxu
Copy link
Member Author

Kubuxu commented Jul 6, 2016

Are you sure that it isn't just measurement error?
On my loaded server I've got: 252/s to 395/s

We should display stdev for all of them.
Also that test might not be benefiting from the cache.

@Kubuxu
Copy link
Member Author

Kubuxu commented Jul 6, 2016

Here are results from my server:

Results Master After % Change
PatchOpsPerSec 14.08 13.93 -1.09%
DirAddOpsPerSec 414.69 437.25 5.44%
DirAddOpsStdev 55.80 27.55 -50.63%
Add10MBTime 3.27 2.80 -14.21%
Add10MBStdev 1.00 0.72 -27.93%

but numbers are all over the place on that server.

@Kubuxu
Copy link
Member Author

Kubuxu commented Jul 6, 2016

This is also what I am getting on my notebook:

 Results          Before  After   % Change
PatchOpsPerSec   56.28   73.34   30.31%
DirAddOpsPerSec  679.71  643.48  -5.33%
DirAddOpsStdev   23.17   61.40   165.04%
Add10MBTime      0.49    0.41    -17.60%
Add10MBStdev     0.49    0.36    -25.44%
Cat1MBTime       10.38   8.38    -19.33%
Cat1MBStdev      2.13    0.94    -55.90%

I can't explain the DirAddOps.

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu
Copy link
Member Author

Kubuxu commented Jul 6, 2016

Results from C1 Scaleway, no variable load:

Results          Before  After   % Change
PatchOpsPerSec   66.91   69.92   4.49%
DirAddOpsPerSec  52.65   52.21   -0.84%
DirAddOpsStdev   1.53    1.71    11.81%
Add10MBTime      2.75    2.84    3.07%
Add10MBStdev     0.05    0.04    -11.82%
Cat1MBTime       140.50  131.18  -6.63%
Cat1MBStdev      19.61   13.90   -29.10%

@whyrusleeping
Copy link
Member

Hrm... that doesnt appear to provide significant improvement.

@Kubuxu
Copy link
Member Author

Kubuxu commented Jul 6, 2016

Yeah, I think that in many cases there will be RAM/Disk kernel level cache hit.
I propose we bump it from 0.4.3 milestone, and rethink. Maybe in this case simpler (and with lower flat cost) LRU cache will work better.

@whyrusleeping
Copy link
Member

@Kubuxu sounds good to me

@Kubuxu Kubuxu removed this from the ipfs-0.4.3 milestone Jul 6, 2016
@Kubuxu Kubuxu added status/in-progress In progress need/community-input Needs input from the wider community and removed need/review Needs a review labels Jul 6, 2016
@whyrusleeping whyrusleeping added the status/blocked Unable to be worked further until needs are met label Aug 4, 2016
@whyrusleeping
Copy link
Member

marking as blocked on better metrics and benchmarking

@whyrusleeping whyrusleeping removed the status/in-progress In progress label Aug 18, 2016
@whyrusleeping whyrusleeping added the status/in-progress In progress label Sep 14, 2016
@Kubuxu Kubuxu added status/ready Ready to be worked status/deferred Conscious decision to pause or backlog and removed status/in-progress In progress status/ready Ready to be worked labels Sep 28, 2016
@whyrusleeping
Copy link
Member

We should add more tests to ipfs-whatever, and then make it easy to run those benchmarks on various backing datastores.

@whyrusleeping
Copy link
Member

@Kubuxu I'm gonna close this, if we decide later that adding the arc cache here will improve perf significantly then we can revive the branch from the archive

@whyrusleeping whyrusleeping deleted the feature/blocks-cache-arc branch March 25, 2017 04:03
@whyrusleeping whyrusleeping removed status/deferred Conscious decision to pause or backlog labels Mar 25, 2017
@Kubuxu
Copy link
Member Author

Kubuxu commented Mar 25, 2017

As you removed this branch it won't be in archive. I will repush.

@Kubuxu Kubuxu restored the feature/blocks-cache-arc branch March 25, 2017 04:07
@whyrusleeping
Copy link
Member

Sorry, I really like the delete branch button. Its fantastic. So much fun to click

@Kubuxu Kubuxu deleted the feature/blocks-cache-arc branch March 25, 2017 04:12
@Kubuxu Kubuxu restored the feature/blocks-cache-arc branch March 25, 2017 04:12
@Kubuxu Kubuxu deleted the feature/blocks-cache-arc branch August 10, 2017 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/community-input Needs input from the wider community status/blocked Unable to be worked further until needs are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants