[WIP] Implement Basic LRU Cache for merkledag nodes. #2890

kevina · 2016-06-22T17:58:31Z

Closes #2849.

This is still a work in progress.

kevina · 2016-06-22T18:15:47Z

@whyrusleeping this is a very basic implementation. I could use some guidance in what you are looking for. (1) Currently only Get's are cached, I am thinking we should also populate the cache when adding nodes. (2) I am thinking that maybe the 2Q or ARC cache from the golang-lru package (https://godoc.org/github.com/hashicorp/golang-lru) would be a better fit, to protect against large reads (or adds) flushing the cahce. (3) I am not sure the best place in the pipeline to put this.

kevina · 2016-06-22T18:22:07Z

CC @Kubuxu

whyrusleeping · 2016-06-22T18:29:21Z

merkledag/merkledag_cache.go

+	cache *lru.Cache
+}
+
+func CloneNode(nd * Node) *Node {


you can just use nd.Copy()

That is an overkill. A shallow copy is all that is needed. I don't think any sort of copying should be necessary as it is in general bad practice to modify an object returned from the cache.

I disagree. We cannot guarantee that callers will not choose to modify nodes they get back from the DAGService. This happens very frequently, see the object patch commands, the dagmodifier, the pinset commands, mfs, and a few other places. If a deep copy becomes a perf issue, we can address it then.

general bad practice to modify an object returned from the cache.

This might be true, but the caller has no idea if the object they are getting was returned from a cache

Okay will use nd.Copy().

whyrusleeping · 2016-07-02T02:42:42Z

@kevina i have a small benchmark here: https://github.com/whyrusleeping/ipfs-whatever

That tests the performance of object operations. This PR should help with that. Mind running some numbers?

kevina · 2016-07-02T03:27:33Z

@whyrusleeping sure, what do you want me to do?

whyrusleeping · 2016-07-02T03:31:28Z

If i could just see some before and after numbers that would be nice. Just start a daemon, run the program a few times and record the outputs.

kevina · 2016-07-02T04:36:15Z

@whyrusleeping right now there is no improvement, I take it only the patch operations should see any real improvements?

I will look into the reason, it would be helpful if you could add some additional benchmarks for workflows you expect to be improved from this cache.

whyrusleeping · 2016-07-02T04:37:57Z

@kevina will do

whyrusleeping · 2016-07-02T04:38:05Z

Maybe try caching on puts too

kevina · 2016-07-02T04:40:44Z

@whyrusleeping yeah caching on puts is something I will try.

I will assume that is is only the patch operations should see an improvement and will disable to add from the benchmark for now.

kevina · 2016-07-02T05:09:52Z

After caching puts I get a near 100% hit rate and am still not seeing any improvements. I notice a lot of disk/io so maybe it is the writing of the new patched node that is slowing everything down.

whyrusleeping · 2016-07-02T05:37:42Z

@kevina i pushed another benchmark to that set that creates directories with a few hundred child nodes. That should show some different numbers.

kevina · 2016-07-02T06:01:07Z

Okay, now we are seeing improvements. Preliminary results: DirAddOpsPerSec before = 47.5 after = 200.

kevina · 2016-07-02T06:10:59Z

I spoke too soon. It made the mistake of not running the initial test more than once. It seams like the OS-level disk cache is having more effect than the DAG cache.

whyrusleeping · 2016-07-05T22:38:12Z

@kevina i'm seeing some respectable perf improvement here.

	master	dag-cache	improvement
PatchOpsPerSec	1632	1752	7%
DirAddOptsPerSec	647	670	3.5%
Add10MB	71.2ms	74.5ms	4.9%
Add10MBStdDev	11.85ms	9.4ms	24%

I particularly like the reduction in the standard deviation of add times. That basically tells me the cache is working nicely as the deviation most likely comes from disk contention

whyrusleeping · 2016-07-05T22:45:55Z

@kevina could you rebase this on master?

kevina · 2016-07-05T22:47:13Z

I'm not sure I would agree that these are respectable improvements. I did not measure the Add10MB as I didn't see how my cache would improve that.

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

kevina · 2016-07-05T23:15:32Z

@whyrusleeping I just rebased on master.

At this point I am not convinced that caching Merkledag nodes globally is necessary a good idea. I am particular worried about the extra copies being made. Especially for 256kb leaf nodes. It might make sense to only cache nodes that contains at least one link. Otherwise the functionally will overlap with the OS cache as possible the block cache.

I did not report such small improvements because I did not see them as statically significant.

For me a cache should improve performance by 50% or more to be worth the overhead. But then again I don't have a lot of experience in this area.

whyrusleeping · 2016-07-05T23:30:45Z

@kevina I agree that we probably dont want to cache leaf nodes, but i've run the tests again on master and i'm seeing much nicer patch ops per second improvement (20-30%)

If you still feel its not worth the overhead, thats fair. But i'd say a 10% improvement is a huge deal in any case (getting 10% improvement in IOPS at my previous company got you a raise).

I've updated ipfs-whatever to be able to generate nicer output between runs:

$ ipfs.master daemon &
$ ipfs-whatever > before.json
$ kill ipfs
$ ipfs.patch daemon &
$ ipfs-whatever --before=before.json

whyrusleeping · 2016-07-05T23:36:16Z

I also just realized that the first numbers i posted an hour ago were run on an in memory tmpfs, not my SSD. (and not my spinning rusty disks either)

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

kevina · 2016-07-06T01:04:21Z

@whyrusleeping, I just pushed a commit to only cache non-leaf nodes (i.e. nodes with at least one link). I didn't bother to report results before because there was so much variance between runs that any improvements where lost in the noise.

kevina · 2016-07-06T01:56:46Z

@whyrusleeping, I am not getting consistent results:

Results  Before  After  % Change

PatchOpsPerSec   420.68  448.91  6.71%
DirAddOpsPerSec  222.52  232.15  4.33%
Add10MBTime      127.06  130.10  2.40%
Add10MBStdev     52.25   51.71   -1.03%

Results  Before  After  % Change

PatchOpsPerSec   439.79  514.01  16.88%
DirAddOpsPerSec  223.23  235.97  5.71%
Add10MBTime      129.81  114.21  -12.02%
Add10MBStdev     53.05   4.29    -91.92%

Results  Before  After  % Change

PatchOpsPerSec   461.58  446.38  -3.29%
DirAddOpsPerSec  239.52  234.18  -2.23%
Add10MBTime      121.79  119.97  -1.50%
Add10MBStdev     31.81   14.28   -55.11%

Here is the script I used:

set -m
ipfs.master daemon --offline &
sleep 5
./ipfs-whatever > before.json
kill %1
sleep 5
ipfs.cache daemon --offline  &
sleep 5
./ipfs-whatever --before=before.json
kill %1

The repo is on a ssd drive with Datastore.NoSync enabled.

I would agree that a 10% improvement would be significant, but only if it was reproducible. At this point I maintain the view that caching dag nodes may not be worthwhile. However, I will be happy to work on this anyway however I would have a hard time tuning it until we can find a workload where the cache makes a significant and reproducible improvement.

Kubuxu · 2016-07-06T06:54:36Z

In my opinion testing those optimizations on our nice SSD enabled dev machines misses the aim of the optimizations.

While doing those optimizations we should think about loaded down, HDD servers.
(I will run your branch on mine and post the results).

Kubuxu · 2016-07-06T20:47:51Z

Two runs:

Results          Before  After   % Change
PatchOpsPerSec   73.59   70.75   -3.86%
DirAddOpsPerSec  52.93   53.92   1.85%
DirAddOpsStdev   0.36    0.37    0.76%
Add10MBTime      2.76    2.91    5.58%
Add10MBStdev     0.08    0.15    95.93%
Cat1MBTime       184.27  232.20  26.01%
Cat1MBStdev      9.60    11.46   19.31%

and

Results          Before  After   % Change
PatchOpsPerSec   66.91   70.64   5.57%
DirAddOpsPerSec  52.65   53.83   2.25%
DirAddOpsStdev   1.53    1.93    25.90%
Add10MBTime      2.75    2.84    3.35%
Add10MBStdev     0.05    0.12    155.66%
Cat1MBTime       140.50  135.34  -3.67%
Cat1MBStdev      19.61   9.53    -51.42%

and this is on very steady load wise machine.

kevina · 2016-07-06T20:53:01Z

@Kubuxu thanks, I propose we hold off on this until we get a better understanding of where the bottlenecks and also also how much CPU time is used to retrieve the block and decode the protobuf. Otherwise there may not be much benefit over the OS level disk cache.

Kubuxu · 2016-07-06T20:53:31Z

Ok, bumping it off the 0.4.3

kevina · 2016-08-24T16:23:52Z

@whyrusleeping (like #2942) I am marking this as blocked. I think we need better metrics to determine where exactly caching will be the most beneficial. If you disagree fell free to remove and readjust the labels.

whyrusleeping · 2016-08-24T16:45:04Z

@kevina good call

whyrusleeping · 2017-03-25T04:02:16Z

@kevina I'm gonna close this for now. If we decide that this will improve performance later on we can pull the branch from the archive and revive the PR

kevina · 2017-03-25T04:07:37Z

That's fine my me.

whyrusleeping reviewed Jun 22, 2016
View reviewed changes

whyrusleeping added this to the Ipfs 0.4.3 milestone Jun 23, 2016

kevina force-pushed the dag-cache branch from 7591d50 to 8dd32e7 Compare July 2, 2016 05:09

Implement Basic LRU Cache for merkledag nodes.

78620e9

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

kevina force-pushed the dag-cache branch from 8dd32e7 to 78620e9 Compare July 5, 2016 22:51

Only cache non-leaf nodes.

384c527

License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

kevina force-pushed the dag-cache branch from 554da97 to 384c527 Compare July 6, 2016 00:37

Kubuxu removed this from the ipfs-0.4.3 milestone Jul 6, 2016

kevina added need/community-input Needs input from the wider community status/blocked Unable to be worked further until needs are met labels Aug 24, 2016

whyrusleeping added the status/in-progress In progress label Sep 14, 2016

Kubuxu removed the status/in-progress In progress label Sep 28, 2016

whyrusleeping closed this Mar 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Implement Basic LRU Cache for merkledag nodes. #2890

[WIP] Implement Basic LRU Cache for merkledag nodes. #2890

kevina commented Jun 22, 2016 •

edited by Kubuxu

Loading

kevina commented Jun 22, 2016

kevina commented Jun 22, 2016

whyrusleeping Jun 22, 2016

kevina Jun 22, 2016 •

edited

Loading

whyrusleeping Jun 22, 2016

kevina Jun 22, 2016

whyrusleeping commented Jul 2, 2016

kevina commented Jul 2, 2016

whyrusleeping commented Jul 2, 2016

kevina commented Jul 2, 2016

whyrusleeping commented Jul 2, 2016

whyrusleeping commented Jul 2, 2016

kevina commented Jul 2, 2016

kevina commented Jul 2, 2016 •

edited

Loading

whyrusleeping commented Jul 2, 2016

kevina commented Jul 2, 2016

kevina commented Jul 2, 2016

whyrusleeping commented Jul 5, 2016

whyrusleeping commented Jul 5, 2016

kevina commented Jul 5, 2016

kevina commented Jul 5, 2016

whyrusleeping commented Jul 5, 2016

whyrusleeping commented Jul 5, 2016

kevina commented Jul 6, 2016

kevina commented Jul 6, 2016

Kubuxu commented Jul 6, 2016

Kubuxu commented Jul 6, 2016 •

edited

Loading

kevina commented Jul 6, 2016

Kubuxu commented Jul 6, 2016

kevina commented Aug 24, 2016

whyrusleeping commented Aug 24, 2016

whyrusleeping commented Mar 25, 2017

kevina commented Mar 25, 2017

[WIP] Implement Basic LRU Cache for merkledag nodes. #2890

[WIP] Implement Basic LRU Cache for merkledag nodes. #2890

Conversation

kevina commented Jun 22, 2016 • edited by Kubuxu Loading

kevina commented Jun 22, 2016

kevina commented Jun 22, 2016

whyrusleeping Jun 22, 2016

Choose a reason for hiding this comment

kevina Jun 22, 2016 • edited Loading

Choose a reason for hiding this comment

whyrusleeping Jun 22, 2016

Choose a reason for hiding this comment

kevina Jun 22, 2016

Choose a reason for hiding this comment

whyrusleeping commented Jul 2, 2016

kevina commented Jul 2, 2016

whyrusleeping commented Jul 2, 2016

kevina commented Jul 2, 2016

whyrusleeping commented Jul 2, 2016

whyrusleeping commented Jul 2, 2016

kevina commented Jul 2, 2016

kevina commented Jul 2, 2016 • edited Loading

whyrusleeping commented Jul 2, 2016

kevina commented Jul 2, 2016

kevina commented Jul 2, 2016

whyrusleeping commented Jul 5, 2016

whyrusleeping commented Jul 5, 2016

kevina commented Jul 5, 2016

kevina commented Jul 5, 2016

whyrusleeping commented Jul 5, 2016

whyrusleeping commented Jul 5, 2016

kevina commented Jul 6, 2016

kevina commented Jul 6, 2016

Kubuxu commented Jul 6, 2016

Kubuxu commented Jul 6, 2016 • edited Loading

kevina commented Jul 6, 2016

Kubuxu commented Jul 6, 2016

kevina commented Aug 24, 2016

whyrusleeping commented Aug 24, 2016

whyrusleeping commented Mar 25, 2017

kevina commented Mar 25, 2017

kevina commented Jun 22, 2016 •

edited by Kubuxu

Loading

kevina Jun 22, 2016 •

edited

Loading

kevina commented Jul 2, 2016 •

edited

Loading

Kubuxu commented Jul 6, 2016 •

edited

Loading