Skip to content

Commit

Permalink
Statically cache the String value of g_internal_field for a small per…
Browse files Browse the repository at this point in the history
…formance gain (#4863)

* Statically cache the String value of g_internal_field for a small performance gain

* Fix lint

* Update src/jsi/jsi_class.hpp

Co-authored-by: Mathias Stearn <redbeard0531@gmail.com>

* Update src/jsi/jsi_class.hpp

Co-authored-by: Mathias Stearn <redbeard0531@gmail.com>

* Change to empty string for internal field name

Co-authored-by: Mathias Stearn <redbeard0531@gmail.com>
  • Loading branch information
2 people authored and kraenhansen committed Sep 15, 2022
1 parent a4639f4 commit 205167f
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 6 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
## vNext (TBD)

### Enhancements
* None
* Small improvement to performance by caching JSI property String object [#4863](https://github.com/realm/realm-js/pull/4863)

### Fixed
* <How to hit and notice issue? what was the impact?> ([#????](https://github.com/realm/realm-js/issues/????), since v?.?.?)
Expand Down Expand Up @@ -36,7 +36,7 @@
<!-- * Using Realm Core vX.Y.Z -->
<!-- * Upgraded Realm Core from vX.Y.Z to vA.B.C -->

## 10.21.0 (2022-09-12)
## 10.21.0 (2022-09-09)

### Enhancements
* Automatic handling backlinks for schema migrations where a class/object type changes to being embedded. ([realm/realm-core#5737](https://github.com/realm/realm-core/pull/5737))
Expand Down
20 changes: 16 additions & 4 deletions src/jsi/jsi_class.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,10 @@ inline void copyProperty(JsiEnv env, const fbjsi::Object& from, const fbjsi::Obj
defineProperty(env, to, name, *prop);
}

inline constexpr const char g_internal_field[] = "__Realm_internal";
// The name used for the property on the JS object which stores the reference to the corresponding C++ object.
// We use an empty string as testing showed it was 1% faster with JSC and 4% faster with Hermes than using
// an actual string, and also has the benefit that it is not a valid Realm object key name.
inline constexpr const char g_internal_field[] = "";

template <typename T>
using ClassDefinition = js::ClassDefinition<js::realmjsi::Types, T>;
Expand Down Expand Up @@ -240,6 +243,10 @@ class ObjectWrap {
// Also, may need to suppress destruction.
inline static std::optional<JsiFunc> s_ctor;

// Cache the JSI String instance representing the field name of the internal
// C++ object for the lifetime of the current env, as this is a hot path
inline static std::optional<fbjsi::String> s_js_internal_field_name;

/**
* @brief callback for invalid access to index setters
* Throws an error when a users attemps to write to an index on a type that
Expand Down Expand Up @@ -310,9 +317,10 @@ class ObjectWrap {
.asFunction(env));

js::Context<js::realmjsi::Types>::register_invalidator([] {
// Ensure the static constructor is destructed when the runtime goes away.
// This is to avoid the reassignment of s_ctor throwing because the runtime has disappeared.
// Ensure the static constructor and JSI String reference are destructed when the runtime goes away.
// This is to avoid reassignment and destruction throwing because the runtime has disappeared.
s_ctor.reset();
s_js_internal_field_name.reset();
});

for (auto&& [name, prop] : s_type.static_properties) {
Expand Down Expand Up @@ -490,7 +498,11 @@ class ObjectWrap {

static Internal* get_internal(JsiEnv env, const JsiObj& object)
{
auto internal = object->getProperty(env, g_internal_field);
if (REALM_UNLIKELY(!s_js_internal_field_name)) {
s_js_internal_field_name = fbjsi::String::createFromAscii(env, g_internal_field);
}

auto internal = object->getProperty(env, *s_js_internal_field_name);
if (internal.isUndefined()) {
// In the case of a user opening a Realm with a class-based model,
// the user defined constructor will get called before the "internal" property has been set.
Expand Down

0 comments on commit 205167f

Please sign in to comment.