Cesarvr/mixed implementation #3501

cesarvr · 2021-01-13T13:18:00Z

What, How & Why?

Mixed Type

It provide integration between the realm::Mixed type into our Realm JS SDK.

Usage

As mentioned before the Mixed type basically allows the user to delegate the type inference at runtime to Realm-JS, so instead of defining the types in the schema like this:

const Schema = {
    name: 'Test',
    properties: {
     a: 'int',
     b: 'string', 
     c: 'bool'
    }
}

Which require the user to remember the types, with the new Mixed type this is not longer necessary because this type deduct the type automatically, you can use the below schema interchangeably with the one above:

const Schema = {
    name: 'Test',
    properties: {
     a: 'mixed',
     b: 'mixed', 
     c: 'mixed'
    }
}

This in combination with Dictionaries can facilitate a more agile way of development where the users are not constrained by initial schema rules or mistakes.

Performance

To make sure we don’t degrade the user experience, we tested the performance by creating two tables with 2 Million rows using a wide variety of types, then we use the fixed schema as the reference for the performance.

Mixed 207 seconds
Schema 175 seconds

We found out that the Mixed type is just 18% slower than the non-Mixed.

Profiling

To understand this slowdown we can take a look at the profiling information:

We expend the majority of CPU time is spend deducting types and parsing complex types as we can see in the figure below:

Another explanation for the performance degradation vs the fixed schema option is the time spent by the CPU dealing with the Mixed type in Realm Core:

Those are the points where we need to focus for further speed improvements.

This closes # RJS-544

☑️ ToDos

📝 Changelog entry
📝 Compatibility label is updated or copied from previous entry
🚦 Tests
📝 Public documentation PR created or is not necessary
💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

typescript definitions file is updated
jsdoc files updated
Chrome debug API is updated if API is available on React Native

…ough a need of polish is required.

… the node_value_return.hpp class

…ry reusable.

bmunkholm · 2021-01-13T14:08:39Z

Just a comment to the motivation. It's not (originally) intended to defer type decisions but to allow storing mixed values when there is a need for that.

cesarvr · 2021-01-13T15:23:26Z

Just a comment to the motivation. It's not (originally) intended to defer type decisions but to allow storing mixed values when there is a need for that.

Thanks for the comment @bmunkholm, but I think I used the word inference, instead of defer, meaning that the framework will figure out the type for you.

And of course the side effect of that is that it will store everything you trow at it as long as we support the type.

bmunkholm · 2021-01-14T08:06:23Z

This in combination with Dictionaries can facilitate a more agile way of development where the users are not constrained by initial schema rules or mistakes.

It doesn't have any effect to the feature itself (that's all good), I'm merely pointing out that above is not really the motivation for this feature or how we will promote the use of it. It shouldn't make you "lazy" or defer decisions about your data types you want.

CHANGELOG.md

src/common/type_deduction.hpp

src/js_mixed.hpp

src/js_object_accessor.hpp

src/js_types.hpp

src/node/node_type_deduction.hpp

fronck · 2021-01-19T11:12:07Z

src/common/type_deduction.hpp

@@ -0,0 +1,37 @@
+////////////////////////////////////////////////////////////////////////////
+//
+// Copyright 2016 Realm Inc.


[nitpick] *2021 ;)

fronck · 2021-01-19T11:14:58Z

src/js_mixed.hpp

+
+#pragma once
+
+#include <common/type_deduction.hpp>


[question] Would you not need the #if REALM_PLATFORM_NODE guard here as well?

fronck · 2021-01-19T11:20:49Z

src/js_mixed.hpp

+};
+
+template <typename Context, typename Value, typename Utils>
+class HandleString : public MixedWrapper<Context, Value> {


[code style] I think these classes would be better named Mixed* (e.g., MixedString) rather than Handle*. Just to make it clear that they deal with Mixed values only.
Alternatively, wrapping them in a mixed namespace or, even better, nested as classes within `MixedWrapper.

nirinchev · 2021-01-19T11:31:23Z

Another note on the description of the PR: you can't use those two schemas interchangeably because:

You'll need a migration for a local Realm.
That'll be a destructive change for a synchronized Realm which is not allowed.

docs/realm.js

src/js_mixed.hpp

nirinchev · 2021-01-19T11:35:14Z

src/js_mixed.hpp

+template <typename Context, typename Value, typename Utils>
+class HandleBinary : public MixedWrapper<Context, Value> {
+    Mixed wrap(Context context, Value const &value) {
+        auto owned_binary_data = Utils::to_binary(context, value);


BinaryData and StringData are very similar in Core - they don't hold a reference to their contents. So it's surprising that you need the cache for the string case, but not in the binary case.

Good catch, thanks for the heads up 👍

nirinchev · 2021-01-19T11:40:03Z

src/js_mixed.hpp

+    Mixed wrap(Context context, Value const &value) {
+        auto date = Utils::to_date(context, value);
+
+        double milliseconds = Utils::to_number(context, date);


[q] Isn't it better to std::move the date (and the ts at the end) variables here?

src/js_mixed.hpp

nirinchev · 2021-01-19T11:42:46Z

src/js_mixed.hpp

+        if (strategy == nullptr) {
+            throw std::runtime_error(
+                "The " + TypeDeduction::realm_typeof(mixed.get_type()) +
+                " value is not supported for the mixed type.");
+        }


If this is hit, it would be a bug in the SDK, wouldn't it? There should be no type in Mixed that can't be wrapped to Value.

That's a good observation, but even in that unlikely case it would be nice to fail gracefully.

If we did want to fail gracefully, I would suggest rewording the error message to something that makes it clear to the developer that this is a bug on our end and they should file a ticket.

nirinchev · 2021-01-19T11:44:05Z

src/node/node_type_deduction.hpp

+        if (value.IsUndefined()) {
+            return type_TypedLink;
+        }


Why is undefined returned as typedlink?

nirinchev · 2021-01-19T11:44:20Z

src/node/node_type_deduction.hpp

+        if (value.IsObject()) {
+            return type_Link;
+        }


Shouldn't this be TypedLink?

nirinchev · 2021-01-19T11:44:57Z

src/node/node_type_deduction.hpp

+            return type_Link;
+        }
+
+        return type_Link;


This seems like a weird default - perhaps hitting this should be an error or return type_Invalid?

fronck

A few nitpicks about the explicitness of the Mixed handler types, and a possible memory leak we need to look at.

Also, some tests are needed.

Otherwise, good work.

src/js_mixed.hpp

Co-authored-by: Nikola Irinchev <[email protected]>

…-js into cesarvr/mixed_implementation

… <-> Javascript

This reverts commit 960ce9f.

cesarvr and others added 13 commits December 18, 2020 10:22

initial

7307ddb

Initial commit, the mixed type for NodeJS should be done by now, alth…

7def4a2

…ough a need of polish is required.

improving cosmetics

bd90a2f

removing debugging lines

80d73a3

Improving performance from 25% CPU utilization to approx 6%

edf359b

Added Mixed support for [Binary, ObjectId, Decimal]

68a236d

starting work with the mixed->date type

c4c4449

Adding TimeStamp

4127450

Adding support for additional types and also plugin the Mixed type in…

c8203a8

… the node_value_return.hpp class

Adding deduction class for NodeJS runtime, the existing one is not ve…

5149501

…ry reusable.

Improving the code and removing unncessary boilerplate.

e68070f

Merge branch 'develop' into cesarvr/mixed_implementation

74eddd3

Adding changelogs

bacb278

cesarvr requested review from kneth and fronck January 13, 2021 13:18

kneth reviewed Jan 14, 2021

View reviewed changes

Adding license headers, cleaning up comments, fixing wrong formatting

94674a7

cesarvr mentioned this pull request Jan 16, 2021

Datatype: UUID #3482

Merged

23 tasks

fronck reviewed Jan 19, 2021

View reviewed changes

nirinchev reviewed Jan 19, 2021

View reviewed changes

fronck suggested changes Jan 19, 2021

View reviewed changes

src/js_mixed.hpp Outdated Show resolved Hide resolved

cesarvr and others added 3 commits January 19, 2021 13:33

Update src/js_mixed.hpp

27fb859

Co-authored-by: Nikola Irinchev <[email protected]>

renaming and fixing the licenses date

7910bbd

Checking binary data storage

6def231

cesarvr added 3 commits January 19, 2021 18:46

Merge branch 'cesarvr/mixed_implementation' of github.com:realm/realm…

9e1dfa3

…-js into cesarvr/mixed_implementation

Adding test to the Mixed type

d282efe

Adding a failiure test to the Mix

c930bf2

cesarvr requested a review from steffenagger January 21, 2021 13:46

cesarvr added 3 commits January 22, 2021 08:55

Adding a new type system to cover all the types we usually use. Realm…

038f74c

… <-> Javascript

Merge branch 'develop' into cesarvr/mixed_implementation

b46306e

Update CHANGELOG.md

5c11100

cesarvr merged commit 960ce9f into develop Jan 25, 2021

cesarvr deleted the cesarvr/mixed_implementation branch January 25, 2021 09:59

kneth added a commit that referenced this pull request Mar 10, 2021

Revert "Cesarvr/mixed implementation (#3501)"

14db5de

This reverts commit 960ce9f.

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cesarvr/mixed implementation #3501

Cesarvr/mixed implementation #3501

cesarvr commented Jan 13, 2021 •

edited

Loading

bmunkholm commented Jan 13, 2021

cesarvr commented Jan 13, 2021 •

edited

Loading

bmunkholm commented Jan 14, 2021

fronck Jan 19, 2021 •

edited

Loading

fronck Jan 19, 2021 •

edited

Loading

fronck Jan 19, 2021 •

edited

Loading

nirinchev commented Jan 19, 2021

nirinchev Jan 19, 2021

cesarvr Jan 20, 2021

nirinchev Jan 19, 2021

nirinchev Jan 19, 2021

cesarvr Jan 20, 2021

nirinchev Jan 20, 2021

nirinchev Jan 19, 2021

nirinchev Jan 19, 2021

nirinchev Jan 19, 2021

fronck left a comment


		#pragma once

		#include <common/type_deduction.hpp>

Cesarvr/mixed implementation #3501

Cesarvr/mixed implementation #3501

Conversation

cesarvr commented Jan 13, 2021 • edited Loading

What, How & Why?

Mixed Type

Usage

Performance

Profiling

☑️ ToDos

bmunkholm commented Jan 13, 2021

cesarvr commented Jan 13, 2021 • edited Loading

bmunkholm commented Jan 14, 2021

fronck Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

fronck Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

fronck Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

nirinchev commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fronck left a comment

Choose a reason for hiding this comment

cesarvr commented Jan 13, 2021 •

edited

Loading

cesarvr commented Jan 13, 2021 •

edited

Loading

fronck Jan 19, 2021 •

edited

Loading

fronck Jan 19, 2021 •

edited

Loading

fronck Jan 19, 2021 •

edited

Loading