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

Consider usage of zero copying approach in async client #151

Open
kptfh opened this issue Dec 6, 2019 · 3 comments
Open

Consider usage of zero copying approach in async client #151

kptfh opened this issue Dec 6, 2019 · 3 comments

Comments

@kptfh
Copy link

kptfh commented Dec 6, 2019

Here is the two points where byte[] get copied:

  1. NettyCommand.writeByteBuffer
    Avoid byteBuffer.writeBytes(command.dataBuffer, 0, command.dataOffset);
    Try to wrap command.dataBuffer with io.netty.buffer.Unpooled.wrappedBuffer()

  2. NioCommand.writeCommand
    Avoid usage of byteBuffer.put(command.dataBuffer, 0, command.dataOffset);
    Try to use command.dataBuffer directly instead

@BrianNichols
Copy link
Member

The buffer copy to byte[] was implemented for these reasons:

  1. Parsing byte[] responses can be the same for both Netty and NIO. ByteBuf and ByteBuffer are two completely separate classes.

  2. Parsing byte[] is faster than parsing ByteBuffer. I have not benchmarked ByteBuf with byte[] parsing, but I assume there is a difference here too.

@kptfh
Copy link
Author

kptfh commented Dec 17, 2019

Hi @BrianNichols !
My issue not about parsing/reading response but about writing/serializing async command.
First the command serialized into byte array
Then

  1. For Netty you create ByteBuf and copy byte array into byte buffer but there is an option to wrap existing byte[] array with io.netty.buffer.Unpooled.wrappedBuffer() to avoid copying
  2. For Nio you copy byte array into ByteBuffer but there is also an option to wrap byte array with ByteBuffer

@BrianNichols
Copy link
Member

The option to wrap a byte[] in a ByteBuffer only exists for a non-direct heap ByteBuffer in NIO. The client uses direct ByteBuffers because they use contiguous memory that can't be moved by the jvm. This is an important performance consideration for TCP stacks that are embedded in the OS kernel.

If a non-direct heap ByteBuffer were used, the jvm would likely copy the data from it's heap to a contiguous memory location before applying the socket write. Ultimately, it's a question of whether this copy should be explicit or not.

Netty also has the concept of heap (newHeapBuffer()) vs direct (newDirectBuffer()) byte buffers. I suspect the same principal applies in this case.

If you have client benchmarks that indicate wrapped byte buffers are faster for socket operations, I would be willing to modify the current implementation.

ijusti pushed a commit to ijusti/aerospike-client-java that referenced this issue Mar 28, 2023
* Spring Cache Aerospike improvements:
1. TTL configuration support in AerospikeCacheManager.
2. Added integration tests.
3. Code cleanup & Apache license copyrights.

* add missing implementations

* Replace AerospikeClient with IAerospikeClient.

* Support clear() method.

* Added a test for "cache should not evict" scenario.

* Move AerospikeSerializingCache functionality to AerospikeCache class.

* Remove unnecessary casting.

* Reformat imports.

* Change access levels.

* Refactor:
1. Remove usage of deprecated method "addCache" in getCache.
2. Use external Aerospike cache utils instead of duplicate code.
3. Code cleanup.

* add license header; add private constructor

* Added documentation for AerospikeCache.

* 1. Fix: GetAerospikeCache creates the cache's namespace with the given name.
2. Return Cache instead of AerospikeCache in getMissingCache.

* Added tests to cover Aerospike cache clear() scenarios.

* Refactor:
1. Using AerospikeCacheConfiguration to keep all cache level configuration.
2. New AerospikeCache name that matches an AerospikeCacheConfiguration (the name no longer represents an Aerospike namespace, namespace is now part of the cache configuration).
3. Added Builder pattern to the cache manager and cache configuration to reduce the number of constructors.
4. Modify Aerospike cache tests to the new design.
5. Throwable instead of Exception.

* Wrap get() method with Cache.ValueRetrievalException instead of log.warn message.

* In case the key already exists return the existing value.
Else, write the new given key-value and return null.

* Added documentation for AerospikeCache methods.

* Fix: adding another second to thread.sleep when clearing cache (build via git actions sometimes fails cause clear() was not completed).

* Fix: remove clear cache test since clearing cache is async method and nothing guarantees that it will finish before the thread sleep duration finishes which causes git actions build to fail.

* Fix: call AerospikeOperations count() method with set name only (no need to provide the class).

* Avoid passing nulls in constructors.

* 1. Remove Builder pattern from AerospikeCacheManager, add the missing relevant constructors.
2. Avoid passing nulls to constructors.
3. Code cleanup - modify docs and variable names.

* Refactor:
change configuration structure (providing a namespace is mandatory).
default set is null (write directly to the namespace).
default expiration is 0 (Aerospike Server's default).

* 1. Changing "cache with TTL" test to use a configured cache instead of another cache manager.
2. Adding another test to cover two cache managers scenario.

* Change set and expirationInSeconds to final.

Co-authored-by: yrizhkov <[email protected]>
Co-authored-by: Eugene R <[email protected]>
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

No branches or pull requests

2 participants