chore(java): Disallow writing meta classdef when obj is null

[LiangliangSui]

What does this PR do?

When obj is null, if shareMeta is enabled, no need to write meta classdef

Related issues

Does this PR introduce any user-facing change?

• Does this PR introduce any public API change?
• Does this PR introduce any binary protocol compatibility change?

Benchmark

[chaokunyang]

The deserialization will still read class meta, but the offset will be -1, then the read will fail. Could you add an UT for such case?

[LiangliangSui]

Done.

CI failure is not related to the current changes, fix it in #1691.

[chaokunyang]

The code looks good to me. But do we really to optimize this? When value is null, the ClassDef list is empty, only one byte are written in such cases

[LiangliangSui]

The code looks good to me. But do we really to optimize this? When value is null, the ClassDef list is empty, only one byte are written in such cases

There will be two more write overheads (putInt32, writeVarUint32Small7), which is also a small problem I found while reading the code. If there is no negative impact, I think it can be merged.

[LiangliangSui]

Hi @chaokunyang , could you reply to this PR?

[chaokunyang]

This is an internal, could we keep it inside this class

[LiangliangSui]

Also used in ClassResolver https://github.com/apache/fury/pull/1686/files#diff-849c034dbc915d01d33d09221ec8c78b290ac53a845e3197ab648ec05bea480aR1427-R1430

[chaokunyang]

How about checking meta list size and remove this part of code from class resolver into Fury

[LiangliangSui]

I don't think it's necessary. Reading classDefOffset and reading numClassDefs should be what readClassDefs should do. We shouldn't distribute what readClassDefs should do to other functions.

Why would we do this? Just to move Constants.INVALID_OFFSITE to Fury?

[chaokunyang]

I don't think it's necessary. Reading classDefOffset and reading numClassDefs should be what readClassDefs should do. We shouldn't distribute what readClassDefs should do to other functions.

Why would we do this? Just to move Constants.INVALID_OFFSITE to Fury?

When object is not null, but the type is internal types such pritimitive type, string or ArrayList/HashMap, the meta list will be empty too, which can be optimized as the null value

[LiangliangSui]

but the type is internal types such pritimitive type

Not only internal types, but also the meta list of registered classes(classInfo.classId != NO_CLASS_ID) is empty.

How about checking meta list size and remove this part of code from class resolver into Fury

  private void write(MemoryBuffer buffer, Object obj) {
    int startOffset = buffer.writerIndex();
    boolean shareMeta = config.isMetaShareEnabled();
    if (shareMeta) {
      buffer.writeInt32(INVALID_OFFSITE); // preserve 4-byte for nativeObjects start offsets.
    }
    // reduce caller stack
    boolean isNull = refResolver.writeRefOrNull(buffer, obj);
    if (!isNull) {
      ClassInfo classInfo = classResolver.getOrUpdateClassInfo(obj.getClass());
      classResolver.writeClass(buffer, classInfo);
      writeData(buffer, classInfo, obj);
    }
    // **Just determine the meta list size.**
    if (shareMeta && !isNull && !serializationContext.getMetaContext().writingClassDefs.isEmpty()) {
      buffer.putInt32(startOffset, buffer.writerIndex());
      classResolver.writeClassDefs(buffer);
    }
  }

Is it possible to determine the meta list size at the end?

[LiangliangSui]

but the type is internal types such pritimitive type

Not only internal types, but also the meta list of registered classes(classInfo.classId != NO_CLASS_ID) is empty.

How about checking meta list size and remove this part of code from class resolver into Fury

  private void write(MemoryBuffer buffer, Object obj) {
    int startOffset = buffer.writerIndex();
    boolean shareMeta = config.isMetaShareEnabled();
    if (shareMeta) {
      buffer.writeInt32(INVALID_OFFSITE); // preserve 4-byte for nativeObjects start offsets.
    }
    // reduce caller stack
    boolean isNull = refResolver.writeRefOrNull(buffer, obj);
    if (!isNull) {
      ClassInfo classInfo = classResolver.getOrUpdateClassInfo(obj.getClass());
      classResolver.writeClass(buffer, classInfo);
      writeData(buffer, classInfo, obj);
    }
    // **Just determine the meta list size.**
    if (shareMeta && !isNull && !serializationContext.getMetaContext().writingClassDefs.isEmpty()) {
      buffer.putInt32(startOffset, buffer.writerIndex());
      classResolver.writeClassDefs(buffer);
    }
  }

Is it possible to determine the meta list size at the end?

@chaokunyang Any other suggestions? If so, please let me know.

[chaokunyang]

the meta list of registered classes(classInfo.classId != NO_CLASS_ID) is empty.

For the meta list of registered classes(classInfo.classId != NO_CLASS_ID), we still need to write meta, this needs being done before we set scopde meta share as the default compatible mode

[chaokunyang]

We can check meta size before writing meta

[LiangliangSui]

the meta list of registered classes(classInfo.classId != NO_CLASS_ID) is empty.

For the meta list of registered classes(classInfo.classId != NO_CLASS_ID), we still need to write meta, this needs being done before we set scopde meta share as the default compatible mode

I mean if the current class has been registered, the information of the current class will not be written back to meta.

public void writeClass(MemoryBuffer buffer, ClassInfo classInfo) {
  if (classInfo.classId == NO_CLASS_ID) { // no class id provided.
    // use classname
    if (metaContextShareEnabled) {
      buffer.writeByte(USE_CLASS_VALUE_FLAG);
      // FIXME(chaokunyang) Register class but not register serializer can't be used with
      //  meta share mode, because no class def are sent to peer.
      writeClassWithMetaShare(buffer, classInfo);
    } else {
      // if it's null, it's a bug.
      assert classInfo.packageNameBytes != null;
      metaStringResolver.writeMetaStringBytesWithFlag(buffer, classInfo.packageNameBytes);
      assert classInfo.classNameBytes != null;
      metaStringResolver.writeMetaStringBytes(buffer, classInfo.classNameBytes);
    }
  } else {
    // use classId
    buffer.writeVarUint32(classInfo.classId << 1);
  }
}

[LiangliangSui]

We can check meta size before writing meta

Done

[chaokunyang]

I perfer not expose this variable. It's an internal implementation, we'd better keep it private

[LiangliangSui]

Hi @chaokunyang , could you please take a look?

[LiangliangSui]

The above comments has been completed, please help review again.

[LiangliangSui]

@chaokunyang please help review this

[chaokunyang]

LGTM