Hash ode map json deduplication#11
Conversation
drewjj
left a comment
There was a problem hiding this comment.
Just a few questions. Looks good, though.
Michael7371
left a comment
There was a problem hiding this comment.
LGTM, just noticed an outdated comment.
| List<KeyValue<String, OdeMessageFrameData>> mapDeduplicationResults = outputOdeMapData | ||
| .readKeyValuesToList(); | ||
|
|
||
| // validate that only 3 messages make it through |
There was a problem hiding this comment.
chore: stale comment, update to 4 messages.
| int oldHash = Objects.hash(lastMessage.toString()); | ||
| int newHash = Objects.hash(newMessage.toString()); | ||
|
|
||
| if(oldHash != newHash){ |
There was a problem hiding this comment.
issue: the hash can have collisions.
suggestion: test object equality instead. Either using 'toString' if there is some reason to do that (not sure if there is):
lastMessage.toString().equals(newMessage.toString())or use the 'equals' method defined on OdeMessageFrameData (by the @EqualsAndHashCode annotation on OdeData):
lastMessage.equals(newMessage)There was a problem hiding this comment.
The two objects being compared are different objects, with different nested elements. ToString is used as a quick way to ensure I am comparing the content of the object as opposed to the memory pointers.
Using the message hash's instead of the messages themselves was selected for performance reasons. From my testing it takes about 1 ms to compare the message hashes from the strings compared to about 3ms to perform string comparison on the strings. While a collision is possible it is quite unlikely given that MAP messages change very infrequently.
In the event that a collision did happen, the impact is fairly minimal. The original MAP would be retained for at most 1 additional hour, before the new MAP message takes over anyway.
There was a problem hiding this comment.
Agree that .equals won't work because on closer inspection, the equals method isn't implemented throughout the object graph. But for the string comparison, I tried to reproduce your benchmark, but got a different result. toString comparison is slightly faster than hashing and comparing the hashes, including the most common case where the majority of comparisons are equal. About 0.3 ms per comparison any way you slice it on my machine.
Benchmark with toString inside the test loop:
Benchmark [equal=50%, changed=50%] (hash compare): iterations=10000, equals=5000, elapsedMs=3080, perComparisonMs=0.308
Benchmark [equal=50%, changed=50%] (string equals): iterations=10000, equals=5000, elapsedMs=3014, perComparisonMs=0.3014
Benchmark [equal=1%, changed=99%] (hash compare): iterations=10000, equals=100, elapsedMs=3069, perComparisonMs=0.3069
Benchmark [equal=1%, changed=99%] (string equals): iterations=10000, equals=100, elapsedMs=3046, perComparisonMs=0.3046
Benchmark [equal=99%, changed=1%] (hash compare): iterations=10000, equals=9900, elapsedMs=3080, perComparisonMs=0.308
Benchmark [equal=99%, changed=1%] (string equals): iterations=10000, equals=9900, elapsedMs=2949, perComparisonMs=0.2949
But the time is dominated by the toString serialization step, whether hashed or not. The benchmark with toString precomputed outside the test loop shows hashing definitely makes the comparison slower:
Benchmark with toString precomputed:
Benchmark [equal=50%, changed=50%] (hash compare): iterations=10000, equals=5000, elapsedMs=19, perComparisonMs=0.0019
Benchmark [equal=50%, changed=50%] (string equals): iterations=10000, equals=5000, elapsedMs=6, perComparisonMs=6.0E-4
Benchmark [equal=1%, changed=99%] (hash compare): iterations=10000, equals=100, elapsedMs=27, perComparisonMs=0.0027
Benchmark [equal=1%, changed=99%] (string equals): iterations=10000, equals=100, elapsedMs=3, perComparisonMs=3.0E-4
Benchmark [equal=99%, changed=1%] (hash compare): iterations=10000, equals=9900, elapsedMs=20, perComparisonMs=0.002
Benchmark [equal=99%, changed=1%] (string equals): iterations=10000, equals=9900, elapsedMs=11, perComparisonMs=0.0011
Using the equals method probably would be faster if it were fully implemented (maybe a TODO for Q3) but i'd recommend full string comparison for now because it's more correct and not slower.
iyourshaw
left a comment
There was a problem hiding this comment.
see comment, full string comparison seems to be faster
This PR updates the logic in the OdeMapJson deduplication topology so that it will forward a new MAP message if the new map message is different from the previous message. Previously, a new OdeMapJson message would only be forwarded if it happens more than 1 hour after the preceeding message. After this update, a new OdeMapJson message will also be forwarded if it has different contents than the previous MAP message. For example, if the new message has different lane ID's or geometries it will be forwarded regardless of how long after the previous message the new message occurred. This update changes the OdeMapJson topology logic to be more in line with how the ProcessedMap and other topologies operate. As with the ProcessedMap topology, this update ignores changes to the ASN.1, odeReceivedAt, and MOY fields since these fields are expected to change frequently and do not necessarily correspond to a change in the intersection geometry.