[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 |
||
| 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 This defeats the purpose of using binary types in the first place. Is there a way to solve Thanks, [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 |
| 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 ] |
| 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! |