[MDSAL-170] Populate _value in IpAddressBinary union lazily Created: 26/May/16  Updated: 09/Mar/18  Resolved: 01/Jun/16

Status: Verified
Project: mdsal
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug
Reporter: Lori Jakab Assignee: Robert Varga
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

Operating System: All
Platform: All


External issue ID: 5970

 Description   

The lispflowmapping project switched using YANG modeled binary representation for IP addresses [0] internally in the southbound plugin, where it is rarely necessary to have access to string representation of the addresses. I did some profling before and after the change, to assess the gains in performance. To my surprise, code using the union of binary IPv4 and IPv6 (i.e., IpAddressBinary) suffered a serious performance regression, instead of improving.

For example, we switched this code:

public static IpAddress getIpAddressFromInetAddress(InetAddress address)

{ return IetfInetUtil.INSTANCE.ipAddressFor(address); }

to this:

public static IpAddressBinary getIpAddressBinaryFromInetAddress(InetAddress address)

{ return IpAddressBinaryBuilder.getDefaultInstance(address.getAddress()); }

Using YourKit with sampling, for 20 minutes, the first takes ~2s total time, while the second takes ~150s. The reason for the regression is the constructor of the union generated code, which looks like this:

public IpAddressBinary(Ipv4AddressBinary _ipv4AddressBinary)

{ super(); this._ipv4AddressBinary = _ipv4AddressBinary; this._ipv6AddressBinary = null; this._value = BaseEncoding.base64().encode(_ipv4AddressBinary.getValue()).toCharArray(); }

public IpAddressBinary(Ipv6AddressBinary _ipv6AddressBinary)

{ super(); this._ipv6AddressBinary = _ipv6AddressBinary; this._ipv4AddressBinary = null; this._value = BaseEncoding.base64().encode(_ipv6AddressBinary.getValue()).toCharArray(); }

Those 150s are spent in the encode() method. I assume this is part of the solution for MDSAL-131 [1], where I only reviewed the proposed solution patch for functionality, and not performance. Note that we don't have this issue with the base types Ipv4AddressBinary and Ipv6AddressBinary, their classes don't even contain an encode() method, only decode() for passing a string as an argument to the constructor.

This defeats the purpose of using binary types in the first place. Is there a way to solve MDSAL-131 in a way that doesn't affect performance? I guess for the IPv4 and IPv6 binary base types the base64 encoding is only done when really necessary. Could that be done for the union too?

Thanks,
-Lori

[0] https://git.opendaylight.org/gerrit/gitweb?p=lispflowmapping.git;a=blob;f=mappingservice/lisp-proto/src/main/yang/odl-inet-binary-types.yang;h=9706197c7b2f4481396af002fffa1efe13ad3b09;hb=HEAD
[1] https://bugs.opendaylight.org/show_bug.cgi?id=5446



 Comments   
Comment by Robert Varga [ 28/May/16 ]

Argh, union types again. The problem is implementation interplay between UnionTemplate.xtend and its superclass ClassTemplate.xtend around initialization of _value field.

In normal classes this field is final, as it really holds the value. This is not the case for unions, as the value is actually held by one of the constituent fields.

Unfortunately ClassTemplate's requirement to have _value initialized is forcing UnionTemplate to eagerly encode it – which is done in the beautiful bottom part of UnionTemplate#unionConstructors().

The interplay has also further bad effects, as union classes take _value into account in their toString(), hashCode() and equals() methods – and they should not, as this information is already included via the constituent types.

To fix this, unions' _value field must not be final, but rather default to null. It should be initialized on first access via getValue() by looking for the first non-null constituent and convering it to char[]. This means the conversion will be done lazily, fixing the performance issue observed. I am not sure how to do this cleanly.

Also hashCode()/equals()/toString() must not touch the _value in any way, as that would either force its instantiation (shifting the performance problem) or lead to inconsistent results. This can be achieved by not adding "value" via GeneratedTOBuilder.add

{Equals,Hash,ToString}

Property() if the target is an union.

Comment by Robert Varga [ 28/May/16 ]

Patch to remove _value from hashCode/equals/toString: https://git.opendaylight.org/gerrit/39554.

Comment by Robert Varga [ 28/May/16 ]

The patch above should address all concerns.

Comment by Robert Varga [ 28/May/16 ]

This is a major performance regression, Be-3 should not ship without a fix.

Comment by Robert Varga [ 31/May/16 ]

be: https://git.opendaylight.org/gerrit/39657

Comment by Lori Jakab [ 01/Jun/16 ]

Apart from checking the generated code, I did some profiling, and while getIpAddressFromInetAddress() still shows up in sampled output with the previously measured ~2 seconds, getIpAddressBinaryFromInetAddress() is completely missing from repeated sampled profiling sessions, which is what I was looking for

Thanks!

Generated at Wed Feb 07 20:08:50 UTC 2024 using Jira 8.20.10#820010-sha1:ace47f9899e9ee25d7157d59aa17ab06aee30d3d.