[pigeon] Adds FFI and JNI support to swift and kotlin generators#11337
[pigeon] Adds FFI and JNI support to swift and kotlin generators#11337tarrinneal wants to merge 85 commits intoflutter:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces FFI and JNI support to the Swift and Kotlin generators, respectively, enabling more performant native interop. It also overhauls the data class equality and hashing logic across all supported languages for better consistency and performance, including proper handling of NaN and signed zero for floating-point types. While the new equality checks are a significant improvement, the implementation for map/dictionary comparison has a quadratic time complexity (O(n^2)), which could be a performance bottleneck for large maps.
| if (a instanceof Map && b instanceof Map) { | ||
| Map<?, ?> mapA = (Map<?, ?>) a; | ||
| Map<?, ?> mapB = (Map<?, ?>) b; | ||
| if (mapA.size() != mapB.size()) { | ||
| return false; | ||
| } | ||
| for (Map.Entry<?, ?> entryA : mapA.entrySet()) { | ||
| Object keyA = entryA.getKey(); | ||
| Object valueA = entryA.getValue(); | ||
| boolean found = false; | ||
| for (Map.Entry<?, ?> entryB : mapB.entrySet()) { | ||
| Object keyB = entryB.getKey(); | ||
| if (pigeonDeepEquals(keyA, keyB)) { | ||
| Object valueB = entryB.getValue(); | ||
| if (pigeonDeepEquals(valueA, valueB)) { | ||
| found = true; | ||
| break; | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| if (!found) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The deep equality check for Map instances has a time complexity of O(n^2) due to the nested loops used for comparison. This could lead to significant performance degradation for large maps.
A more performant approach should be considered. For example, if the keys' hashCode and equals methods are consistent with pigeonDeepEquals, you could iterate through one map's entries and perform lookups on the second map, which would be much faster. If that's not a safe assumption, an alternative to the nested loop would be to build a mutable copy of one map's entries and remove them as they are matched. This would reduce the complexity from O(n^2) to O(n) on average, at the cost of higher memory usage.
| if (a is Map<*, *> && b is Map<*, *>) { | ||
| return a.size == b.size && | ||
| a.all { (b as Map<Any?, Any?>).contains(it.key) && deepEquals(it.value, b[it.key]) } | ||
| if (a.size != b.size) return false | ||
| for (entry in a) { | ||
| val key = entry.key | ||
| var found = false | ||
| for (bEntry in b) { | ||
| if (deepEquals(key, bEntry.key)) { | ||
| if (deepEquals(entry.value, bEntry.value)) { | ||
| found = true | ||
| break | ||
| } else { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| if (!found) return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
The deepEquals implementation for Map has a time complexity of O(n^2) due to the nested loops. This could become a performance bottleneck when dealing with large maps. It would be beneficial to consider a more efficient algorithm. For instance, if key equality semantics allow, iterating through one map and performing lookups in the other would be more performant. An alternative could be to create a mutable copy of one map's entries and remove them as they are matched to avoid the nested loop structure.
| case (let lhsDictionary, let rhsDictionary) as ([AnyHashable: Any?], [AnyHashable: Any?]): | ||
| guard lhsDictionary.count == rhsDictionary.count else { return false } | ||
| for (lhsKey, lhsValue) in lhsDictionary { | ||
| var found = false | ||
| for (rhsKey, rhsValue) in rhsDictionary { | ||
| if deepEqualsMessages(lhsKey, rhsKey) { | ||
| if deepEqualsMessages(lhsValue, rhsValue) { | ||
| found = true | ||
| break | ||
| } else { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| if !found { return false } | ||
| } | ||
| return true |
There was a problem hiding this comment.
The deepEqualsMessages implementation for Dictionaries exhibits O(n^2) time complexity due to the nested loops. This could lead to performance issues with large dictionaries. A more performant solution should be considered. For example, if the keys conform to Hashable in a way that is consistent with deepEqualsMessages, you could avoid the inner loop and achieve better performance.
| case FL_VALUE_TYPE_MAP: { | ||
| size_t len = fl_value_get_length(a); | ||
| if (len != fl_value_get_length(b)) { | ||
| return FALSE; | ||
| } | ||
| for (size_t i = 0; i < len; i++) { | ||
| FlValue* key = fl_value_get_map_key(a, i); | ||
| FlValue* val = fl_value_get_map_value(a, i); | ||
| gboolean found = FALSE; | ||
| for (size_t j = 0; j < len; j++) { | ||
| FlValue* b_key = fl_value_get_map_key(b, j); | ||
| if (flpigeon_deep_equals(key, b_key)) { | ||
| FlValue* b_val = fl_value_get_map_value(b, j); | ||
| if (flpigeon_deep_equals(val, b_val)) { | ||
| found = TRUE; | ||
| break; | ||
| } else { | ||
| return FALSE; | ||
| } | ||
| } | ||
| } | ||
| if (!found) { | ||
| return FALSE; | ||
| } | ||
| } | ||
| return TRUE; | ||
| } |
There was a problem hiding this comment.
The flpigeon_deep_equals function for FL_VALUE_TYPE_MAP has a time complexity of O(n^2) due to the nested loops. This can be inefficient for large maps. Please consider a more performant implementation.
| if ([a isKindOfClass:[NSDictionary class]] && [b isKindOfClass:[NSDictionary class]]) { | ||
| NSDictionary *dictA = (NSDictionary *)a; | ||
| NSDictionary *dictB = (NSDictionary *)b; | ||
| if (dictA.count != dictB.count) { | ||
| return NO; | ||
| } | ||
| for (id keyA in dictA) { | ||
| id valueA = dictA[keyA]; | ||
| BOOL found = NO; | ||
| for (id keyB in dictB) { | ||
| if (FLTPigeonDeepEquals(keyA, keyB)) { | ||
| id valueB = dictB[keyB]; | ||
| if (FLTPigeonDeepEquals(valueA, valueB)) { | ||
| found = YES; | ||
| break; | ||
| } else { | ||
| return NO; | ||
| } | ||
| } | ||
| } | ||
| if (!found) { | ||
| return NO; | ||
| } | ||
| } | ||
| return YES; | ||
| } |
There was a problem hiding this comment.
The FLTPigeonDeepEquals implementation for NSDictionary has a time complexity of O(n^2) due to the nested loops. This can be inefficient for large dictionaries. A more performant approach should be considered to avoid potential performance bottlenecks.
| template <typename K, typename V> | ||
| bool PigeonInternalDeepEquals(const std::map<K, V>& a, | ||
| const std::map<K, V>& b) { | ||
| if (a.size() != b.size()) { | ||
| return false; | ||
| } | ||
| for (const auto& kv : a) { | ||
| bool found = false; | ||
| for (const auto& b_kv : b) { | ||
| if (PigeonInternalDeepEquals(kv.first, b_kv.first)) { | ||
| if (PigeonInternalDeepEquals(kv.second, b_kv.second)) { | ||
| found = true; | ||
| break; | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| if (!found) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The PigeonInternalDeepEquals implementation for std::map has a time complexity of O(n^2) due to the nested loops. This could lead to performance issues for large maps. A more efficient algorithm should be considered. Since std::map keys are ordered, you could potentially iterate both maps simultaneously for a more performant comparison.
1 participant
still needs documentation, but I think the code is in a reviewable state.