Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Sep 9, 2022

Conversation

tomduncalf
Copy link
Contributor

What, How & Why?

This optimisation yields a small (~5% with Hermes disabled, ~2% with Hermes enabled) performance improvement with our benchmark suite, as it avoids the overhead of repeatedly creating the JSI String object.

image

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 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

Sorry, something went wrong.

@cla-bot cla-bot bot added the cla: yes label Sep 5, 2022
@tomduncalf tomduncalf force-pushed the td/v11-optimize-internal-field branch from a73ce43 to 5d3e50a Compare September 5, 2022 10:40
@tomduncalf tomduncalf marked this pull request as ready for review September 5, 2022 10:40
@@ -490,7 +490,9 @@ class ObjectWrap {

static Internal* get_internal(JsiEnv env, const JsiObj& object)
{
auto internal = object->getProperty(env, g_internal_field);
static const auto js_internal_field = fbjsi::String::createFromAscii(env, g_internal_field);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break on reload since it will continue to use the string from the old env.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right yeah, good point. I guess there's no way round that? It's not a huge gain anyway so I'm happy to close this if that's best

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it a class-static like s_ctor with the same logic to set it on module load and reset on em teardown. This isnt a perfect solution because it assumes at most one env at a time, but it will at least work as well as the rest of the codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in e32fc5b

@tomduncalf
Copy link
Contributor Author

Waiting on #4869 for lint to pass

@kraenhansen kraenhansen force-pushed the v11 branch 2 times, most recently from bf94457 to a388208 Compare September 7, 2022 18:10
@tomduncalf tomduncalf force-pushed the td/v11-optimize-internal-field branch from e32fc5b to 0b6099f Compare September 9, 2022 08:51
src/jsi/jsi_class.hpp Outdated Show resolved Hide resolved
src/jsi/jsi_class.hpp Outdated Show resolved Hide resolved
Tom Duncalf and others added 3 commits September 9, 2022 11:13
Co-authored-by: Mathias Stearn <redbeard0531@gmail.com>
Co-authored-by: Mathias Stearn <redbeard0531@gmail.com>
@tomduncalf tomduncalf force-pushed the td/v11-optimize-internal-field branch from 1e159f6 to 1ed1d1e Compare September 9, 2022 11:44
@tomduncalf
Copy link
Contributor Author

Ignoring known failure on Android

@tomduncalf tomduncalf merged commit 0ea9085 into v11 Sep 9, 2022
@tomduncalf tomduncalf deleted the td/v11-optimize-internal-field branch September 9, 2022 14:15
kraenhansen pushed a commit that referenced this pull request Sep 9, 2022
…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>
kraenhansen pushed a commit that referenced this pull request Sep 9, 2022
…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>
kraenhansen pushed a commit that referenced this pull request Sep 9, 2022
…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>
kraenhansen pushed a commit that referenced this pull request Sep 13, 2022
…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>
kraenhansen pushed a commit that referenced this pull request Sep 15, 2022
…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>
takameyer pushed a commit that referenced this pull request Sep 15, 2022
…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>
kraenhansen pushed a commit that referenced this pull request Oct 18, 2022
…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>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants