Add caching to HeaderChain struct #5403

adrianbrink · 2017-04-05T12:20:31Z

This is the initial PR for adding caching to the HeaderChain struct.

spelling fixes in ethcore-light
add reference to docs for LRU cache
add member variable cache on HeaderChain

Left todo:

use the cache in the methods

parity-cla-bot · 2017-04-05T12:20:32Z

It looks like @adrianbrink hasn'signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.ethcore.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

rphmeier · 2017-04-05T12:21:21Z

ethcore/light/src/client/header_chain.rs

@@ -408,7 +414,7 @@ impl HeaderChain {
 		}
 	}

-	/// Get the nth CHT root, if it's been computed.
+	/// Get the nth CHT root, if it has been computed.


technically not wrong before but reads better here :)

rphmeier · 2017-04-05T12:22:21Z

ethcore/light/src/net/mod.rs

@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.

-//! PIP Protocol Version 1 implementation.
+//! PLP Protocol Version 1 implementation.


No, it really is PIP...

maybe I'll just change the name if it's really confusing :)

But isn't it Parity Lightlient Protocol? I have to say that I find it pretty confusing :-)

Also reversed.

rphmeier · 2017-04-05T12:23:42Z

Thanks! The key method to look at is block_header for utilizing the cache.

rphmeier · 2017-04-06T14:54:32Z

ethcore/light/src/client/header_chain.rs

+				return cached
+			} else {
+				match self.db.get(self.col, &hash) {
+					Ok(val) => val.map(|x| x.to_vec()).map(encoded::Header::new),


Should write the encoded header into the cache on success.
You can have a different match arm for Ok(Some(header_bytes)) and Ok(None) to handle this cleanly

adrianbrink · 2017-04-06T18:59:43Z

I've rebased and now update the cache.

It looks a build verbose due to the fact that insert_block_header() doesn't return the inserted item and hence the extended function block in and_then().

rphmeier · 2017-04-06T19:29:54Z

ethcore/light/src/client/header_chain.rs

+				None => {
+					match self.db.get(self.col, &hash) {
+						Ok(dbValue) => {
+							dbValue.map(|x| x.to_vec()).map(encoded::Header::new)


last thing: in Rust we usually use snake_case style. So dbValue should be db_value

rphmeier · 2017-04-06T19:32:39Z

Okay, LGTM except for the capitalization issue

rphmeier · 2017-04-07T08:37:53Z

ethcore/light/src/client/service.rs

@@ -56,6 +60,8 @@ pub struct Service {
 impl Service {
 	/// Start the service: initialize I/O workers and client itself.
 	pub fn start(config: ClientConfig, spec: &Spec, path: &Path) -> Result<Self, Error> {
+		let cache = Arc::new(Mutex::new(Cache::new(Default::default(), Duration::hours(6))));


This should be given a cache, which should be the same one given to OnDemand and RPC in parity/run.rs

adrianbrink · 2017-04-07T15:44:24Z

@rphmeier Rebased and fixed up parity/run.rs to now use the same Cache. However, currently we are passing the cache size down but I don't think it is used. I can try and fix that in another PR.

rphmeier · 2017-04-07T15:57:02Z

Thanks!

LGTM, seems like some random other changes have gotten mixed into the diff, though.
Also, the CI doesn't appear to have run...

adrianbrink · 2017-04-07T16:10:59Z

Anything that I should fix? Aren't those other changes from me rebasing?

gavofyork · 2017-04-07T16:11:26Z

will push this as a branch in our repo to get the fire

gavofyork · 2017-04-07T16:12:30Z

see #5428

rphmeier · 2017-04-07T17:11:36Z

Looks like there are some test build failures.
Can you make changes so running test.sh builds and completes successfully? Maybe set CARGO_INCREMENTAL=1 for faster compilation.

adrianbrink · 2017-04-07T19:15:38Z

Just noticed. I'll fix it now and then I'll update the PR. And also, I'll run the test script. Before I only ran the tests for the light_client crate.

adrianbrink · 2017-04-07T19:40:16Z

@gavofyork @rphmeier
The tests in ./test.sh are all passing now locally. Could you trigger a 2nd CI build.

Also, do any of you have a good guide on how to handle rebasing when using a forking approach to git? I have rebased again but it is always messy to rebase against the upstream(parity)/master and then follow that with a rebase against origin/adrian-lightclientcache.

adrianbrink · 2017-04-07T19:44:49Z

Also, is there a way to speed up the tests for the entire parity build?

I'm running them with CARGO_INCREMENTAL=1 ./test.sh on a MBP 13" and it takes around 5 min. :-)

rphmeier · 2017-04-07T19:47:16Z

Usually we just git merge against the latest master rather than rebase because it's clean.

And that's about as fast as the tests get :)

adrianbrink · 2017-04-08T06:21:27Z

Okay, that's good to know. I'll switch to git merge next time :-)

rphmeier reviewed Apr 5, 2017

View reviewed changes

rphmeier added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Apr 5, 2017

rphmeier reviewed Apr 6, 2017

View reviewed changes

rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Apr 6, 2017

rphmeier reviewed Apr 6, 2017

View reviewed changes

rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 6, 2017

rphmeier reviewed Apr 7, 2017

View reviewed changes

rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Apr 7, 2017

rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 7, 2017

gavofyork mentioned this pull request Apr 7, 2017

Adrianbrink lightclientcache branch. #5428

Merged

rphmeier added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Apr 7, 2017

Spelling fixes and link addition about LRU cache in the docs.

e004e05

Adrian Brink added 11 commits April 7, 2017 21:34

Add cache to HeaderChain struct.

8ea25ee

Add caching to block_header()

8a7ca6f

Add header to cache

23c76ca

Clean up

3632a98

Change to snakecase

9552ca7

Add cache to HeaderChain struct.

5e33fe0

Add header to cache

fa88ee1

Ensure that OnDemand and HeaderChain share the same cache

2ce5a65

Ensure that OnDemand and HeaderChain share the same cache

d6bc60f

Fix failing tests

645b8e4

Update light_sync test to include cache dependency

144d6c2

adrianbrink force-pushed the adrian-lightclientcache branch from f954818 to 144d6c2 Compare April 7, 2017 19:38

rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Apr 10, 2017

rphmeier merged commit 95808f5 into openethereum:master Apr 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add caching to HeaderChain struct #5403

Add caching to HeaderChain struct #5403

adrianbrink commented Apr 5, 2017

parity-cla-bot commented Apr 5, 2017

rphmeier Apr 5, 2017

adrianbrink Apr 6, 2017

rphmeier Apr 5, 2017

adrianbrink Apr 5, 2017

adrianbrink Apr 6, 2017

rphmeier commented Apr 5, 2017

rphmeier Apr 6, 2017

adrianbrink commented Apr 6, 2017

rphmeier Apr 6, 2017

rphmeier commented Apr 6, 2017

rphmeier Apr 7, 2017

adrianbrink commented Apr 7, 2017

rphmeier commented Apr 7, 2017

adrianbrink commented Apr 7, 2017

gavofyork commented Apr 7, 2017

gavofyork commented Apr 7, 2017

rphmeier commented Apr 7, 2017 •

edited

Loading

adrianbrink commented Apr 7, 2017

adrianbrink commented Apr 7, 2017 •

edited

Loading

adrianbrink commented Apr 7, 2017

rphmeier commented Apr 7, 2017

adrianbrink commented Apr 8, 2017

Add caching to HeaderChain struct #5403

Add caching to HeaderChain struct #5403

Conversation

adrianbrink commented Apr 5, 2017

parity-cla-bot commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier commented Apr 5, 2017

Choose a reason for hiding this comment

adrianbrink commented Apr 6, 2017

Choose a reason for hiding this comment

rphmeier commented Apr 6, 2017

Choose a reason for hiding this comment

adrianbrink commented Apr 7, 2017

rphmeier commented Apr 7, 2017

adrianbrink commented Apr 7, 2017

gavofyork commented Apr 7, 2017

gavofyork commented Apr 7, 2017

rphmeier commented Apr 7, 2017 • edited Loading

adrianbrink commented Apr 7, 2017

adrianbrink commented Apr 7, 2017 • edited Loading

adrianbrink commented Apr 7, 2017

rphmeier commented Apr 7, 2017

adrianbrink commented Apr 8, 2017

rphmeier commented Apr 7, 2017 •

edited

Loading

adrianbrink commented Apr 7, 2017 •

edited

Loading