New option: reuse-objects for Java generator

[adam-aph]

https://issues.apache.org/jira/browse/THRIFT-2368
For applications serializing and deserializing millions of transactions it is important to avoid unnecessary memory allocations - the allocated and abandoned objects needs to be collected and deleted by GC, which creates "stop-the-world" pauses. The new compiler option forces thrift compiler to generate code which is reusing existing objects for deserialization (this is up to caller to make sure that read data will fit). Without that option compiler generates the same code as originally.

code generated by original compiler (0.9.1):

         case 1: // HEADER
            if (schemeField.type == org.apache.thrift.protocol.TType.STRUCT) {
              struct.header = new com.gtech.ngin.ncp.libs.thrift.binary.BinaryHeaderStruct();
              struct.header.read(iprot);
              struct.setHeaderIsSet(true);
            } else { 
              org.apache.thrift.protocol.TProtocolUtil.skip(iprot, schemeField.type);
            }
            break;
          case 2: // KEYS
            if (schemeField.type == org.apache.thrift.protocol.TType.SET) {
              {
                org.apache.thrift.protocol.TSet _set0 = iprot.readSetBegin();
                struct.keys = new HashSet<com.gtech.ngin.ncp.libs.thrift.binary.BinaryKeyStruct>(2*_set0.size);
                for (int _i1 = 0; _i1 < _set0.size; ++_i1)
                {
                  com.gtech.ngin.ncp.libs.thrift.binary.BinaryKeyStruct _elem2;
                  _elem2 = new com.gtech.ngin.ncp.libs.thrift.binary.BinaryKeyStruct();
                  _elem2.read(iprot);
                  struct.keys.add(_elem2);
                }
                iprot.readSetEnd();
              }
              struct.setKeysIsSet(true);
            } else { 
              org.apache.thrift.protocol.TProtocolUtil.skip(iprot, schemeField.type);
            }
            break;

code generated with enabled "java:reuse-objects" option:

          case 1: // HEADER
            if (schemeField.type == org.apache.thrift.protocol.TType.STRUCT) {
              if (struct.header == null) {
                struct.header = new com.gtech.ngin.ncp.libs.thrift.binary.BinaryHeaderStruct();
              }
              struct.header.read(iprot);
              struct.setHeaderIsSet(true);
            } else {
              org.apache.thrift.protocol.TProtocolUtil.skip(iprot, schemeField.type);
            }
            break;
          case 2: // KEYS
            if (schemeField.type == org.apache.thrift.protocol.TType.SET) {
              {
                org.apache.thrift.protocol.TSet _set0 = iprot.readSetBegin();
                if (struct.keys == null) {
                  struct.keys = new HashSet<com.gtech.ngin.ncp.libs.thrift.binary.BinaryKeyStruct>(2*_set0.size);
                }
                com.gtech.ngin.ncp.libs.thrift.binary.BinaryKeyStruct _elem1 = new com.gtech.ngin.ncp.libs.thrift.binary.BinaryKeyStruct();
                for (int _i2 = 0; _i2 < _set0.size; ++_i2)
                {
                  if (_elem1 == null) {
                    _elem1 = new com.gtech.ngin.ncp.libs.thrift.binary.BinaryKeyStruct();
                  }
                  _elem1.read(iprot);
                  struct.keys.add(_elem1);
                }
                iprot.readSetEnd();
              }
              struct.setKeysIsSet(true);
            } else {
              org.apache.thrift.protocol.TProtocolUtil.skip(iprot, schemeField.type);
            }
            break;

[henrique]

Hi Adam, thanks for the request.
Do you think this new parameter is really necessary?
I would take this change more as a direct improvement since it wouldn't break existing code, would it?
Additional parameters make testing harder...

[adam-aph]

Hi Henrique - I think this new parameter would be required - the patch is not breaking existing code, however it may reveal issues in existing applications (currently the deserialization delivers always newly allocated objects, but when the re-use is switched on, the application needs to remember to properly "clean" all storage objects, otherwise subsequent deserializations will be added on the top of previous ones.
So I think enabling this feature by default for all applications may create headache.

[henrique]

in this case we should have at least unit-tests demonstrating, and testing, the correct usage.
What do you think?

[adam-aph]

unfortunatelly you are right ;)
I can create updated unit test based on existing one for java - but I'm not familiar enough with Thrift to judge what I should use as the base, so I would need some guidance.

[henrique]

That would be great Adam!
Maybe someone else has a better idea, but you can have a look on https://github.com/apache/thrift/blob/master/lib/java/build.xml
perhaps generate ThriftTest.thrift with a different "-out" folder and run the tests there?
Cheers!

[adam-aph]

ok, I added unit tests for this new option
I also updated the Export/Import entries in the manifest (via build.xml), so the bundle (jar) is now more OSGi friendly.

please let me know how do you like these changes.

[asfbot]

Build triggered. Test FAILed.

[jfarrell]

closing per THRIFT-2368