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.

Sign up for GitHub

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

Jump to bottom

Implement nativePerformanceNow to improve Profiler API results #27885

Closed
wants to merge 9 commits into from
Closed

Implement nativePerformanceNow to improve Profiler API results #27885

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
868b947
Implement nativePerformanceNow and create performance polyfill with p…
emilisb Jan 27, 2020
a320543
Override performance.now only if it is not defined
emilisb Jan 27, 2020
67ed052
Use high_resolution_clock for Android nativePerformanceNow and move N…
emilisb Jan 30, 2020
70da200
Do not override global.performance in Systrace, fix "now" check in pe…
emilisb Jan 30, 2020
8556fae
Fix references of polyfill in installPerformanceHooks
emilisb Jan 30, 2020
79defd9
Create global.performance in Systrace if it is undefined
emilisb Jan 30, 2020
c329299
Combine two if's into one
emilisb Jan 30, 2020
9e32105
Use std::chrono::steady_clock
emilisb Feb 11, 2020
d64f79d
Initialize Performance in Core instead of polyfill
emilisb Feb 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions Libraries/polyfills/performance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @format
* @flow strict
* @polyfill
*/

if (!global.performance) {
global.performance = {
now: function() {
return global.nativePerformanceNow();
emilisb marked this conversation as resolved.
Show resolved Hide resolved
},
};
}
7 changes: 7 additions & 0 deletions React/CxxBridge/JSCExecutorFactory.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@
[NSString stringWithUTF8String:message.c_str()]);
};
react::bindNativeLogger(runtime, iosLoggingBinder);

react::PerformanceNow iosPerformanceNowBinder =[]() {
// CACurrentMediaTime() returns the current absolute time, in seconds
return CACurrentMediaTime() * 1000;
};
react::bindNativePerformanceNow(runtime, iosPerformanceNowBinder);
Copy link

Choose a reason for hiding this comment

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

Is there a reason to bind this here (in each executor factory) instead of in somewhere more central like Instance::bindBridge? (I guess that would probably involve changing the API for bindBridge to add another parameter, which would be a breaking change for any other executors that aren't part of RN core). I'm just wondering because it seems like the difference between how we bind nativePerformanceNow is based on the platform we're running on, not the JS VM, so we don't necessarily need this to be in the JS executor factory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed like a good place to use without making any breaking changes. Logging binder is there too.


// Wrap over the original runtimeInstaller
if (runtimeInstaller) {
runtimeInstaller(runtime);
Expand Down
6 changes: 6 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/hermes/reactexecutor/OnLoad.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <react/jni/JReactMarker.h>
#include <react/jni/JSLogging.h>
#include <react/jni/JavaScriptExecutorHolder.h>
#include <react/jni/NativeTime.h>

#include <memory>

Expand Down Expand Up @@ -47,6 +48,11 @@ static void installBindings(jsi::Runtime &runtime) {
static_cast<void (*)(const std::string &, unsigned int)>(
&reactAndroidLoggingHook);
react::bindNativeLogger(runtime, androidLogger);

react::PerformanceNow androidNativePerformanceNow =
static_cast<double (*)()>(
&reactAndroidNativePerformanceNowHook);
react::bindNativePerformanceNow(runtime, androidNativePerformanceNow);
}

class HermesExecutorHolder
Expand Down
6 changes: 6 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/jscexecutor/OnLoad.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <react/jni/JSLogging.h>
#include <react/jni/JavaScriptExecutorHolder.h>
#include <react/jni/ReadableNativeMap.h>
#include <react/jni/NativeTime.h>

#include <memory>

Expand All @@ -30,6 +31,11 @@ class JSCExecutorFactory : public JSExecutorFactory {
static_cast<void (*)(const std::string &, unsigned int)>(
&reactAndroidLoggingHook);
react::bindNativeLogger(runtime, androidLogger);

react::PerformanceNow androidNativePerformanceNow =
static_cast<double (*)()>(
&reactAndroidNativePerformanceNowHook);
react::bindNativePerformanceNow(runtime, androidNativePerformanceNow);
};
return std::make_unique<JSIExecutor>(
jsc::makeJSCRuntime(),
Expand Down
22 changes: 22 additions & 0 deletions ReactAndroid/src/main/jni/react/jni/NativeTime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#include <time.h>
#include "NativeTime.h"

namespace facebook {
namespace react {

double reactAndroidNativePerformanceNowHook() {
struct timespec now;
clock_gettime(CLOCK_MONOTONIC, &now);
emilisb marked this conversation as resolved.
Show resolved Hide resolved

return (now.tv_sec * 1000000000LL + now.tv_nsec) / 1000000.0;
emilisb marked this conversation as resolved.
Show resolved Hide resolved
}

} // namespace react
} // namespace facebook
16 changes: 16 additions & 0 deletions ReactAndroid/src/main/jni/react/jni/NativeTime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#pragma once

namespace facebook {
namespace react {

double reactAndroidNativePerformanceNowHook();

} // namespace react
} // namespace facebook
17 changes: 17 additions & 0 deletions ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,5 +417,22 @@ void bindNativeLogger(Runtime &runtime, Logger logger) {
}));
}

void bindNativePerformanceNow(Runtime &runtime, PerformanceNow performanceNow) {
runtime.global().setProperty(
runtime,
"nativePerformanceNow",
Function::createFromHostFunction(
runtime,
PropNameID::forAscii(runtime, "nativePerformanceNow"),
0,
[performanceNow = std::move(performanceNow)](
jsi::Runtime &runtime,
const jsi::Value &,
const jsi::Value *args,
size_t count) {
return Value(performanceNow());
}));
}

} // namespace react
} // namespace facebook
3 changes: 3 additions & 0 deletions ReactCommon/jsiexecutor/jsireact/JSIExecutor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,5 +135,8 @@ class JSIExecutor : public JSExecutor {
using Logger =
std::function<void(const std::string &message, unsigned int logLevel)>;
void bindNativeLogger(jsi::Runtime &runtime, Logger logger);

using PerformanceNow = std::function<double()>;
void bindNativePerformanceNow(jsi::Runtime &runtime, PerformanceNow performanceNow);
} // namespace react
} // namespace facebook
1 change: 1 addition & 0 deletions rn-get-polyfills.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ module.exports = () => [
require.resolve('./Libraries/polyfills/console.js'),
require.resolve('./Libraries/polyfills/error-guard.js'),
require.resolve('./Libraries/polyfills/Object.es7.js'),
require.resolve('./Libraries/polyfills/performance.js'),
Copy link

Choose a reason for hiding this comment

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

Do we need to have the polyfill? I think we really want to remove polyfills instead of adding more... what if we just added a JS module that people could require normally if they want to use it?

Copy link
Contributor Author

@emilisb emilisb Jan 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I looked through the code, and I noticed that if Systrace is enabled, Libraries/Performance/Systrace.js would override global.performance. What if we load performance module in InitializeCore.js, and combine performance.now with all the methods from Systrace into one module that would define performance?

Edit: nevermind, it would not work, we cannot access nativePerformanceNow binding in this stage.

Copy link

Choose a reason for hiding this comment

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

Why can't we access nativePerformanceNow? It should be installed already, right?

Forgive the ignorant question, but - what would happen if, instead of installing global.nativePerformanceNow, we just actually installed global.performance.now directly instead? Would that be a disaster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why I initially thought we cannot access nativePerformanceNow there. I moved the polyfill to Core instead.

The rest of performance implementation is in JS (it's part of Systrace), so I think it would be better to keep now in JS as well. Also, it's nice to have a fallback in case the executor has no performance.now implementation.

];