Fix some performance issues when initializing the Schema #5404
Conversation
| writer.emitStatement( | ||
| "OsObjectSchemaInfo.Builder builder = new OsObjectSchemaInfo.Builder(\"%s\")", this.simpleClassName); | ||
| "OsObjectSchemaInfo.Builder builder = new OsObjectSchemaInfo.Builder(\"%s\", %s, %s)", | ||
| this.simpleClassName, persistedFields, computedFields); |
There was a problem hiding this comment.
I have this weird idea that you could probably just pass in the metadata as is, that way you wouldn't need to directly pass ints as parameters to the constructor.
Unless of course that is not possible because of below.
There was a problem hiding this comment.
Not entirely sure what you mean? OsObjectSchemaInfo.Builder contains two internal arrays. You mean that those should be constructed outside and then parsed in?
There was a problem hiding this comment.
Actually never mind, there's a lot more to do with these sizes than I thought (JNI)
beeender
left a comment
beeender
left a comment
There was a problem hiding this comment.
there are some more improvements can be done, i will make an other PR. But shall we merge this to releases branch??
|
Next release is going to be from |
|
Yeah, master is fine to me |
| long propertyPtr = Property.nativeCreatePersistedProperty(name, | ||
| Property.convertFromRealmFieldType(type, isRequired), isPrimaryKey, isIndexed); | ||
| persistedPropertyPtrArray[persistedPropertyPtrCurPos] = propertyPtr; | ||
| persistedPropertyPtrCurPos++; |
There was a problem hiding this comment.
why not persistedPropertyPtrArray[persistedPropertyPtrCurPos++] = propertyPtr;
There was a problem hiding this comment.
I prefer this way since i always have problems to understand the operator priority :P
| persistedPropertyList.add(property); | ||
| long propertyPtr = Property.nativeCreatePersistedProperty(name, | ||
| Property.convertFromRealmFieldType(type, isRequired), !Property.PRIMARY_KEY, !Property.INDEXED); | ||
| persistedPropertyPtrArray[persistedPropertyPtrCurPos] = propertyPtr; |
| if (prop->is_primary) { | ||
| object_schema.primary_key = prop->name; | ||
| } | ||
| object_schema.persisted_properties.emplace_back(std::move(*prop)); |
There was a problem hiding this comment.
@beeender I assume the container will free the dynamically allocated pointer when destructed (since it took ownership of the pointer when using the move ctor)
There was a problem hiding this comment.
uh, no. the container will "empty" the Property&& which is *prop here. That means after this line, the prop is still a valid pointer which stores an empty Property.
The memory of the empty Property(pointed by prop) will be freed at line 76.
It will be easy to understand in this way i think:
Property& prop_ref = *prop; // nothing is alloced here
object_schema.persisted_properties.emplace_back(std::move(prop_ref));
// prop_ref has been "emptied" by the move constructor
// but it is still a valid ref since prop is still a valid pointer.
This PR fixes some of the issues found in #5391
3.7.2 - Nexus 6P
1445
1397
1345
1350
1359
Avg: 1379
4.0.0-SNAPSHOT with proxy class fixes
1129
1067
1054
973
1030
Avg: 1051
Savings ~25%
4.0.0-SNAPSHOT with proxy/mediator fixes
1115
974
939
937
950
Avg: 983
Savings ~ 29%
4.0.0-SNAPSHOT with no-alloc/proxy/mediator fixes
988
944
966
959
997
Avg: 971
Savings ~ 30%
1 second to open a Realm still feels like too much though, but this was just low hanging fruits.