Unnecessary instantiation of `clazz` if `includeManifest` is disabled in `KryoSerializerBackend`

[hveiga]

Hello,

I am debugging an issue with Kryo deserialization using Artery and includeManifest is set to false. When deserializing any object I can find that manifest is "" and therefore clazz always becomes a Failure(ClassNotFoundException("")) since it cannot find a proper class for the value "". Since our system is network-heavy, we are seeing this consuming more CPU than expected (throwing exception is usually expensive and should be avoided if possible):

The actual issue seems to be here:

akka-kryo-serialization/akka-kryo-serialization/src/main/scala/io/altoo/akka/serialization/kryo/KryoSerializerBackend.scala

Lines 74 to 84 in c5b2ed2

	override def fromBinary(buf: ByteBuffer, manifest: String): AnyRef = {
	val buffer = getInput(buf)
	val clazz = system.dynamicAccess.getClassFor[AnyRef](manifest)
	if (includeManifest)
	clazz match {
	case Success(c) => kryo.readObject(buffer, c).asInstanceOf[AnyRef]
	case _ => throw new RuntimeException("Object of unknown class cannot be deserialized")
	}
	else
	kryo.readClassAndObject(buffer)
	}

I am happy to open a PR to solve the issue once I get confirmation this should be changed.

[danischroeter]

Hi @hveiga
You're absolutely right - clazz should be initialized only within the if clause! Good catch.
Would be nice if you could supply a PR. Thx!

[hveiga]

I'll do it shortly, thanks for the confirmation!

[hveiga]

Thanks for responding and merging so quick, greatly appreciated. Could we get this change released on 2.5.1 some time soon?

[danischroeter]

Thx for finding the issue - this benefits everybody...
I guess we'll create a bugfix for this one right away - should be in maven central by tomorrow...