-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
cache ipns entries to speed things up a little #1887
Conversation
It would be good to reflect that in a Cache-Control max-age header for /ipns addresses on the gateway. https://github.com/ipfs/go-ipfs/issues/1818 |
47a0685
to
b0e2090
Compare
👍 |
After this merges i encourage people to really start using ipns, and reporting any issues to me. I'm pretty confident in it (post this changeset), but thats not to say it won't have any issues. |
8fa19cf
to
0c59c63
Compare
I did an experiment with the current I have been thinking of doing an experiment with live video streaming in this way. Unfortunately, this change might affect the current update rate negatively. On the other hand, if I were to publish a blog using IPNS, I might be fine with nodes having a stale version of the record for up to an hour. I realize future pubsub support will cover the live streaming use case, but having the ability to do Don’t let this delay merging this PR, but I hope IPNS records will gain a TTL field (like in DNS records) in the future. |
return 0, err | ||
} | ||
|
||
ct := cfg.Ipns.ResolveCacheTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have this in the default config too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can, but we dont need to. It assumes that an empty value means the default value of one minute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought for the sake of documentation ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added to the default config
eol, ok := checkEOL(entry) | ||
if ok && eol.Before(cacheTil) { | ||
cacheTil = eol | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this stuff could get easily wrong, and cache time errors could behavior drastically. i'd propose feeding entry
into r.cacheSet
and put the computation in one place, calculated always from the original data, instead of intermediate values.
i'd support the addition of the --ttl flag for |
👍 to this PR in general. |
er, wait. that option is already there in the form of |
1bc21f0
to
5db2437
Compare
@jbenet do you want |
|
no, dont merge yet. still CRing |
cacheTil := time.Now().Add(r.cachelife) | ||
eol, ok := checkEOL(rec) | ||
if ok && eol.Before(cacheTil) { | ||
cacheTil = eol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
um... this isn't used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an interesting case of go's unused variable error. Its used, then assigned to, then that assigned value is never used.
I'm a bit worried this will hide speed problems enough that they don't get fixed... |
@gamemanj read the comment chain. |
5db2437
to
d93eee0
Compare
@jbenet actually used that variable. |
The
This is the time “No cache” should be a property of the record (i.e. |
@ion1 @gamemanj I also want to point out that live streaming will be done best by pubsub, ipns polling for streaming is equatable to setting your dns records to each frame of the video, one at a time. Not to say that you shouldnt try it, and that ipns should be slow, but thats just not its intended purpose. |
changed the value back to 16, and changed the cache to an lru, with a configurable size. No other DHT parameters changed here, weird things started happening when i tweaked K, so i'm not going to bother until later. |
f9352b3
to
26d7561
Compare
Nice. Is the cache lifetime limit useful in addition to the LRU policy though? How about letting popular records with long TTLs be cached as long as their TTL value allows? You could get the TTL value from the record, fall back to a default of one minute, compute |
Also, the LRU cache implementation does its own locking. |
return | ||
} | ||
|
||
ctx = context.WithValue(ctx, "ipns-publish-ttl", d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 we should enhance the context much more often.
This would allow us to get a trace of where it failed in timeouts instead of a blank "deadline exceeded".
@whyrusleeping, please see my commits in https://github.com/ion1/go-ipfs/commits/feat/ipns-cache The first commit has some fixes for things including the redundant locking and The second commit removes cachelife with the assumption that the LRU policy along with respecting the TTL (and EOL) fields is enough. |
@whyrusleeping I also fixed your tests.. :) Qmduc5TqSk5hWnuvBCDHQMXnPALPZ131Jz9wbU9SYTbjyB |
26d7561
to
4342b8e
Compare
License: MIT Signed-off-by: Johan Kiviniemi <[email protected]>
1bd03c0
to
9957772
Compare
👍 used it already |
I refactored the EOL computation out of |
A bit better: return both the absolute |
Alright. 👍, especially after squashing so one commit doesn’t add cachelife just for another to remove it. |
License: MIT Signed-off-by: Jeromy <[email protected]>
9957772
to
c7fb7ce
Compare
this LGTM |
cache ipns entries to speed things up a little
@whyrusleeping @ion1 @cryptix damn i merged this without realizing-- there's no tests. can we write some? |
This adds a quick minute long cache to ipns entry resolution.
Currently, ipns is pretty much unusable for webpages as each sub-request must re-resolve the entry.
License: MIT
Signed-off-by: Jeromy [email protected]