-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Implement nativePerformanceNow to improve Profiler API results #27885
Changes from 2 commits
868b947
a320543
67ed052
70da200
8556fae
79defd9
c329299
9e32105
d64f79d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/** | ||
* 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 = {}; | ||
} | ||
|
||
/** | ||
* Returns a double, measured in milliseconds. | ||
* https://developer.mozilla.org/en-US/docs/Web/API/Performance/now | ||
*/ | ||
if (typeof global.performance !== 'function') { | ||
global.performance.now = function() { | ||
return global.nativePerformanceNow(); | ||
}; | ||
} |
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 |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, Edit: nevermind, it would not work, we cannot access nativePerformanceNow binding in this stage. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we access Forgive the ignorant question, but - what would happen if, instead of installing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no idea why I initially thought we cannot access The rest of |
||
]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.