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

Caching store tracker #157

Merged
merged 75 commits into from Sep 21, 2015
Merged

Caching store tracker #157

merged 75 commits into from Sep 21, 2015

Conversation

toad
Copy link
Contributor

@toad toad commented Feb 27, 2013

No description provided.

voxsim and others added 30 commits February 15, 2013 20:20
…etStore is lossy, since it only has 5 possible places to put each key, so change howManyBlocks in howManyBlocks*5
…/unregister and take snapshot in pushAllCachingStores
…ExpireSSK and waiting the flushing of the cache in testOverMaximumSize
…ngStores.size()]) to avoid casting from Object, delete pushAllCachingStores call direclty in add
…store-tracker

Conflicts:
	src/freenet/node/Node.java
…re), check for size in pushAll loop without having to lock again.
…e. queuedJob prevents us from queueing two or more jobs to write everything in the future.
@freenet-bot
Copy link

Build results will soon be (or already are) available at: https://jenkins.freenetproject.org/job/freenet-pullreq/33/

@freenet-bot
Copy link

Build results will soon be (or already are) available at: https://jenkins.freenetproject.org/job/freenet-pullreq/35/

@freenet-bot
Copy link

Build results will soon be (or already are) available at: https://jenkins.freenetproject.org/job/freenet-pullreq/36/

@freenet-bot
Copy link

Build results will soon be (or already are) available at: https://jenkins.freenetproject.org/job/freenet-pullreq/48/

@nextgens
Copy link
Contributor

nextgens commented Sep 8, 2013

retest this please

@freenet-bot
Copy link

Build results will soon be (or already are) available at: https://jenkins.freenetproject.org/job/freenet-pullreq/58/

@Thynix
Copy link
Contributor

Thynix commented Mar 23, 2014

What does this do?

@toad
Copy link
Contributor Author

toad commented Mar 23, 2014

Locking enhancements related to lazy writing in the datastore IIRC. There are a couple of bugs for it on the bug tracker...
https://bugs.freenetproject.org/view.php?id=5637
https://bugs.freenetproject.org/view.php?id=5636
IIRC this wasn't merged because it needed testing; get it wrong and things could break badly.

@nextgens
Copy link
Contributor

nextgens commented Aug 6, 2014

Doh; I didn't realize this was pending on me...

I've had a look. CachingFreenetStoreTracker.unregisterCachingFS() is broken locking wise; but imho it doesn't matter in real-world usage as we're not unregistering stuff except when we shutdown... The simplest way to fix it is to ensure that we have the write lock in CachingFreenetStore.close() when we're calling it.

@voxsim can you update the pull request with that single change?

The rest looks good to field-test/merge to me; the other two bugs pointed out by toad are further optimizations.

@ArneBab
Copy link
Contributor

ArneBab commented Sep 21, 2014

Is this still current with purge-db4o?

@toad
Copy link
Contributor Author

toad commented Sep 26, 2014

purge-db4o is irrelevant to this commit. The original pull request was merged, this is optimisations / locking changes.

@ArneBab
Copy link
Contributor

ArneBab commented Oct 12, 2014

any updates on this?

@nextgens
Copy link
Contributor

Not that I know of, are you volunteering to update it with the locking change I've suggested in #157 (comment) ?

I'd be great if that went in

@nextgens
Copy link
Contributor

Ok; so that's done. I suggest we wait for a release with no other major change before it makes it to a released build though...

@ArneBab
Copy link
Contributor

ArneBab commented Nov 23, 2014

wow - thanks a lot! Last week I wanted to ask whether this would really just entail adding another lock-section around the unregister - now I see it’s even easier…

@toad
Copy link
Contributor Author

toad commented Nov 23, 2014

This is risky, are we fairly sure about it? What testing can we do on it?

@ArneBab
Copy link
Contributor

ArneBab commented Nov 26, 2014

The easiest test is a testing release after the big releases are done.

@toad
Copy link
Contributor Author

toad commented Nov 26, 2014

Well if it's been carefully reviewed then go ahead, after thorough prerelease testing. It's just one of the few changes that could actually cause e.g. deadlocks, inability to update.

@Thynix
Copy link
Contributor

Thynix commented Dec 6, 2014

I plan to merge this for 1469.

@nextgens
Copy link
Contributor

bump

@Thynix Thynix merged commit ea4c659 into next Sep 21, 2015
@xor-freenet
Copy link
Contributor

<toad_> how much testing have we had of pre-releases with the caching store tracker stuff?
<toad_> IIRC there was a risk of deadlock if there were bugs, so it needed extensive testing?
<toad_> serious, update-breaking deadlock???
<toad_> has this been tested by testers?

@Thynix
Copy link
Contributor

Thynix commented Dec 9, 2015

People have been running the prereleases, yes. For finding deadlocks review
and static analysis seem a more reliable than casual testing.

On Tue, Dec 8, 2015, 10:30 PM xor-freenet notifications@github.com wrote:

<toad_> how much testing have we had of pre-releases with the caching store tracker stuff?
<toad_> IIRC there was a risk of deadlock if there were bugs, so it needed extensive testing?
<toad_> serious, update-breaking deadlock???
<toad_> has this been tested by testers?


Reply to this email directly or view it on GitHub
#157 (comment).

@toad toad deleted the caching-store-tracker branch December 30, 2015 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants