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

Array based ring, for ketama node locator lookups, for improved performance #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tootedom
Copy link

Hi there

This is pull request for changing the KetamaNodeLocator from using a TreeMap<Long,MemcachedNode> to than of an array based ring lookup.

I noticed that the TreeMap implementation has a few performance related issues:

  • creation of Long objects (due to autoboxing) as a result of the TreeMap being keyed on Long objects
  • lookups on containsKey, get, isEmpty take cpu time.

To avoid the first, whilst improving on 2, I have changed the TreeMap ring implementation to use that of a sorted long[], and implement a binary search for the closest ceil long in the array, against which to hash entries. This avoids the creation of the Long objects, and also only searches for the closest ceil long in the array the once.

The jmh benchmark is showing this implementation of the ring to be more performant.

=======================================
Original KetamaNodeLocator:
=======================================

Benchmark                                   Mode  Cnt  Score   Error   Units
NodeLocatorPerfTest.testKetemaNodeLocator  thrpt   40  0.599 ± 0.013  ops/ms

=======================================
New Array Based Ring KetamaNodeLocator:
=======================================
Benchmark                                       Mode  Cnt  Score   Error   Units
NodeLocatorPerfTest.testCeilingKeyNodeLocator  thrpt   40  1.263 ± 0.032  ops/ms

Here is the output from Java Flight Recorder and Mission Control that show the before and after. With the after now showing that item that impacts performance/takes up CPU to be that of the HashingAlgorithm and not the node lookup in the ring:

Before:

CPU:
screen shot 2015-08-15 at 14 55 44

Allocations:
screen shot 2015-08-15 at 14 56 06


After

CPU (getBytes is from get bytes on the string to byte[] required by the key hashing)
screen shot 2015-08-15 at 14 57 24

Memory: (no more Longs)
screen shot 2015-08-15 at 14 58 00


As a side note I have fixed a concurrency issue in the test case: DummyListenableFuture. It was notifying listeners before setting the content. Also as the listeners are executed on a separate thread, the content needs to be volatile to ensure publication of the value (set in one thread) to the executor thread that the listener is running on.

I also increased the sleep time in QueueOverflowTest to allow the client to recover/reconnect as it was always failing, before even these changes were done. With the increased timeout out the test is working.

@daschl
Copy link

daschl commented Oct 14, 2015

@tootedom wow just saw this - seems a great addition!

As you probably know we need to put changes through gerrit (http://www.couchbase.com/wiki/display/couchbase/Contributing+Changes) - if you want I can guide you through the initial process of setting it up, after that its very painless.

If you just sign the CLA and don't mind I can get it in for you as well - but I'd be great if you could squash into one commit and remove the non-related changes (test fixes). We can get those in separately.

If you don't care at all I can also get it in, but I want you attributed of course.

favila pushed a commit to useshortcut/spymemcached that referenced this pull request Jan 14, 2022
Bumped Maven version from 1.1.1 to 1.1.2 in the pom.xml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants