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

introduce low memory flag #2012

Closed
wants to merge 74 commits into from
Closed

Conversation

whyrusleeping
Copy link
Member

I want to try an experiment. I want to have an ipfs 'low memory mode' that when enabled, limits ipfs's memory usage by turning down various numbers, reduces concurrency, and makes in memory caches smaller.

If anyone has any thoughts on where these changes should be made, feel free to post proposals.

License: MIT
Signed-off-by: Jeromy [email protected]

daviddias and others added 30 commits November 11, 2015 10:04
License: MIT
Signed-off-by: David Dias <[email protected]>
This used to lead to large refcount numbers, causing Flush to create a
lot of IPFS objects, and merkledag to consume tens of gigabytes of
RAM.

License: MIT
Signed-off-by: Jeromy <[email protected]>
OS X sed is documented as "-i SUFFIX", GNU sed as "-iSUFFIX". The one
consistent case seems to be "-iSUFFIX", where suffix cannot empty (or
OS X will parse the next argument as the suffix).

This used to leave around files named `refsout=` on Linux, and was
just confusing.

License: MIT
Signed-off-by: Jeromy <[email protected]>
These secondary copies were never actually queried, and didn't contain
the indirect refcounts so they couldn't become the authoritative
source anyway as is. New goal is to move pinning into IPFS objects.

A migration will be needed to remove the old data from the datastore.
This can happen at any time after this commit.

License: MIT
Signed-off-by: Jeromy <[email protected]>
Pinner had method GetManual that returned a ManualPinner, so every
Pinner had to implement ManualPinner anyway.

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
Platform-dependent behavior is not nice, and negative refcounts are
not very useful.

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

sharness: Don't assume we know all things that can create garbage

License: MIT
Signed-off-by: Jeromy <[email protected]>
WARNING: No migration performed! That needs to come in a separate
commit, perhaps amended into this one.

This is the minimal rewrite, only changing the storage from
JSON(+extra keys) in Datastore to IPFS objects. All of the pinning
state is still loaded in memory, and written from scratch on Flush. To
do more would require API changes, e.g. adding error returns.

Set/Multiset is not cleanly separated into a library, yet, as it's API
is expected to change radically.

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
There was doublewrapping with an unneeded msgio. given that we
use a stream muxer now, msgio is only needed by secureConn -- to
signal the boundaries of an encrypted / mac-ed ciphertext.

Side note: i think including the varint length in the clear is
actually a bad idea that can be exploited by an attacker. it should
be encrypted, too. (TODO)

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
* ID service stream
* make the relay service use msmux
* fix nc tests

Note from jbenet: Maybe we should remove the old protocol/muxer
and see what breaks. It shouldn't be used by anything now.

License: MIT
Signed-off-by: Jeromy <[email protected]>
Signed-off-by: Juan Batiz-Benet <[email protected]>
The addition of a locking interface to the blockstore allows us to
perform atomic operations on the underlying datastore without having to
worry about different operations happening in the background, such as
garbage collection.

License: MIT
Signed-off-by: Jeromy <[email protected]>
This commit improves (fixes) the FetchGraph call for recursively
fetching every descendant node of a given merkledag node. This operation
should be the simplest way of ensuring that you have replicated a dag
locally.

This commit also implements a method in the merkledag package called
EnumerateChildren, this method is used to get a set of the keys of every
descendant node of the given node. All keys found are noted in the
passed in KeySet, which may in the future be implemented on disk to
avoid excessive memory consumption.

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Juan Batiz-Benet <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>

dont GC blocks used by pinner

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

comment GC algo

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

add lock to blockstore to prevent GC from eating wanted blocks

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

improve FetchGraph

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

separate interfaces for blockstore and GCBlockstore

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

reintroduce indirect pinning, add enumerateChildren dag method

License: MIT
Signed-off-by: Jeromy <[email protected]>
License: MIT
Signed-off-by: Jeromy <[email protected]>
jbenet and others added 5 commits November 20, 2015 00:27
if bucket doesnt have enough peers, grab more elsewhere
add closenotify and large timeout to gateway
Add config option for flatfs no-sync
License: MIT
Signed-off-by: Jeromy <[email protected]>
@jbenet jbenet added the status/in-progress In progress label Nov 28, 2015
@whyrusleeping
Copy link
Member Author

My idea here is that we find a bunch of places where we can tweak things to make usage lower, and make a 'binary' switch for them now. Later, once we've matured the option a bit, we can make various 'levels' of memory consumption mode (maybe even a high memory mode) that would affect the same areas of code.

@rht
Copy link
Contributor

rht commented Nov 28, 2015

A global runtime memory limit was proposed in #1482, which can't be enabled until at least go1.6 (golang/go#10460).

@ghost
Copy link

ghost commented Nov 28, 2015

This will be great for embedded devices!

@rht
Copy link
Contributor

rht commented Nov 28, 2015

Never mind embedded devices, even VPS got choked with the ram consumption.

Though there is always ulimit (when in unix).

@whyrusleeping
Copy link
Member Author

right, the point of this PR wont be to set a hard number limit on memory, it will be policy changes to attempt to use less memory.

License: MIT
Signed-off-by: Jeromy <[email protected]>
@rht
Copy link
Contributor

rht commented Nov 28, 2015

concurrentFdDials?

@whyrusleeping
Copy link
Member Author

@rht ooooh, yeah. thats a good one

var concurrentFdDials = 160

func init() {
concurrentFdDials = 80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot if u.LowMemMode

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

facepalm I'm good at this...

License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping whyrusleeping force-pushed the feat/low-mem-experiment branch from baeb949 to c809589 Compare November 30, 2015 03:43
@@ -182,6 +182,9 @@ func (i *cmdInvocation) Run(ctx context.Context) (output io.Reader, err error) {
if u.GetenvBool("DEBUG") {
u.Debug = true
}
if u.GetenvBool("IPFS_LOW_MEM") {
u.LowMemMode = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this should go into the config? ok to start like this i guess

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my thought was to keep this 'unofficial' until we're more confident about it working. Would suck to have a low memory mode that increased memory for some people, lol

@jbenet
Copy link
Member

jbenet commented Dec 1, 2015

great idea! could also

  • make it accept a memory bound and try to stick to that (back to resource constraints)
  • run test suites with low mem on
  • finally add that test with a max memory, and start hiking that lower

@whyrusleeping
Copy link
Member Author

  • make it accept a memory bound and try to stick to that (back to resource constraints)

I definitely want that, but I think it will be best to get this out there to get a better feel for its effect on various workloads first. Then once we have a better understanding, start being smarter

  • run test suites with low mem on

Yep! I'll enable that

  • finally add that test with a max memory, and start hiking that lower

mmmm, that would be nice. definitely on the todo list

@jbenet
Copy link
Member

jbenet commented Dec 4, 2015

Yep! I'll enable that

we should run the test suite over both cases. it may have effects in hiding or showing bugs.

@ghost ghost added topic/perf Performance need/community-input Needs input from the wider community labels Dec 22, 2015
@whyrusleeping whyrusleeping force-pushed the dev0.4.0 branch 2 times, most recently from 68b9745 to b0a8591 Compare December 28, 2015 14:36
@whyrusleeping whyrusleeping deleted the feat/low-mem-experiment branch March 2, 2016 07:08
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/in-progress In progress topic/perf Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants