Chore/upgrade performance module to latest apis #88
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…k#49639) Summary: Pull Request resolved: facebook#49639 Changelog: [internal] This refactors some code in the performance API and Perfetto to avoid doing unnecessary work when not tracing (e.g.: string manipulation for tracks). It also reduces the cost of logging marks/measures to tracing backends if not actively tracing (e.g.: moving the check to outside the lock). Reviewed By: bgirard Differential Revision: D69246772 fbshipit-source-id: 141a13f609f12be7ab8ca00f7aa1892b34770bbb
…#49747) Summary: Pull Request resolved: facebook#49747 I'm adding the `target_compile_reactnative_options` to centralize the management of compilation flags for React Native CMake build. Changelog: [Internal] [Changed] - Reviewed By: javache Differential Revision: D70386746 fbshipit-source-id: aefbd5a073d9fab48d09ebbba6507377ef626d73
Summary: Pull Request resolved: facebook#49745 It turns out we want those flags to be available also inside ReactCommon so I'm moving it there. This will allow us to reference them also inside ReactCommon and will make sure Common does not depend on Android Changelog: [Internal] [Changed] - Reviewed By: javache Differential Revision: D70386742 fbshipit-source-id: 3675c01f5e3f6515af6423d75e3fe145ba3d8936
Summary: Pull Request resolved: facebook#50995 Refactors our previous single-struct representation for `PerformanceEntry` ([see MDN](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry)) as a `std::variant`. This maps closely to the `PerformanceEntry` type inheritance in the web spec, and makes this type substantially cleaner to extend and work with in D73861431. Changelog: [Internal] Reviewed By: rubennorte, rshest Differential Revision: D73860532 fbshipit-source-id: f3b7a7444d2456370620c1a1ba9a43f118cb9730
Summary: Pull Request resolved: facebook#50996 Adds a representation of the [`PerformanceResourceTiming`](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming) event type to the Web Performance subsystem, ahead of integration with our network event reporting. Changelog: [Internal] Reviewed By: rubennorte Differential Revision: D73861431 fbshipit-source-id: 8258058c432727808d30c338bba79ca92a17c1c8
) Summary: Pull Request resolved: facebook#51024 Prevents a dependency cycle in the next diff. Changelog: [Internal] Reviewed By: rubennorte, hoxyq Differential Revision: D73933671 fbshipit-source-id: 79c66cd5f6a32ed7287a2d6e7840a512466306a9
Summary: Pull Request resolved: facebook#51025 (Sparsely) wires up reporting of Network events to the Web Performance subsystem. Our plan is to report to the Web Performance APIs (lightweight timing metadata, here) for all build flavours, and report to CDP (more costly full metadata/previews) in dev/profiling builds. **Notes** - Introduces `PerformanceEntryReporter::unstable_reportResourceTiming` — this will become "stable" when further network events/fields are fully hydrated on Android and iOS. Changelog: [Internal] Reviewed By: rubennorte Differential Revision: D73922341 fbshipit-source-id: bcfc03c3d8a9a286ae72ba00a3313602fb2adea8
Summary: Pull Request resolved: facebook#51389 This is the pre-requisite for the diff on top, which migrates performance-related classes to start using `HighResTimeStamp`. We should be throwing `SyntaxError` if the specified mark name is not buffered. Like Chromium does: {F1978032319} In this diff: - `PerformanceEntryReporter::getMarkTime` is now public, ca be called by `NativePerformance` and returns `std::optional`. - `NativePerformance` is responsible for validating that marks are present in the buffer, if their names are specified in `.measure()` call. - Mark names take precedence over `startTime` and `endTime` values, so if they are specified and not available, then we will throw Error over `jsi` that will be catched on JavaScript side in `Performance.js`. Reviewed By: rubennorte Differential Revision: D74837812 fbshipit-source-id: ca2ba198c4c9e6e2d8d37f852affea667f1c174c
Summary: Pull Request resolved: facebook#51511 There are multiple changes: 1. `PerformanceTracer` class, `TraceEvent` struct are moved to `tracing` namespace. These are parts of the Tracing subsystems of the jsinspector, this should bring more clarity and make things more explicit. 2. Added `Timing.h` class which defines conversion logic from `HighResTimeStamp` to absolute units that are expected by CDP. 3. `PerformanceTracer` will receive timestamps for Performance Web API entries in `HighResTimeStamp`. Also, we will explicilty define a Tracking Clock time origin that will be epoch of the `steady_clock`. This aligns with the approach in Chromium and saves us from aligning custom DOMHighResTimeStamps that can be specified in performance.mark / performance.measure calls: these should not extend the timeline window. I've confirmed that this is the current behavior in Chromium. Reviewed By: rubennorte, huntie Differential Revision: D75185467 fbshipit-source-id: 37444392f12e8c9c4479c47c42b2c4badca7ecfd
facebook#51512) Summary: Pull Request resolved: facebook#51512 Pull Request resolved: facebook#50585 Replaces `DOMHighResTimeStamp` alias completely in `ReactCommon` with `HighResTimeStamp`. `DOMHighResTimeStamp` as a type is now expected to be used only in JavaScript. I didn't update places where we explcitly use `std::chrono::high_resolution_clock`, since it is platform-specific and there is no guarantee that `std::chrono::high_resolution_clock` == `std::chrono::steady_clock`. Also, places that are isolated and not part of the Web Performance APIs, such as Telemetry for Fabric, are not updates as part of this diff. Although these subsystems are also using `std::chrono::steady_clock` as a low-level representation, they are not sharing it with other parts of the React Native core. Reviewed By: rubennorte Differential Revision: D75185613 fbshipit-source-id: 889719368de163e6f529689df6cc16d816fde66c
Summary: Pull Request resolved: facebook#51622 We currently support an optional 3rd params for `target_compile_reactnative_options` which allows to specify a LOG_TAG macro. No one is actually reading that Macro. The only usage would be logging from the Android SDK which we don't explicitely use. Here I'm updating our build to specify a LOG_TAG as `ReactNative` for all the targets without allowing to customize it as it just complicates our build setup. Changelog: [Internal] [Changed] - Reviewed By: mdvacca Differential Revision: D75445577 fbshipit-source-id: a426ce77ba6d1dfd0800e874d9f7838bfdc5b877
Summary: Pull Request resolved: facebook#52463 Changelog: [internal] This adds Fantom tests for `performance.eventCounts`. Reviewed By: huntie Differential Revision: D77860881 fbshipit-source-id: 26b9ef56b9c610cbad7011bc0adde27251fda909
…acebook#52586) Summary: Pull Request resolved: facebook#52586 Changelog: [internal] This fixes a "bug" (or spec-compliance issue) in the `performance.measure` method where `end` and `duration` couldn't be used together (only `start` and `duration`, and `start` and `end` could be used). This also refactors the API to be "JS-first", preparing the native module methods to support passing instances of entries to fix other issues. Verified performance impact: `mark` is slightly regressed (~7% slower) due to an additional JSI call to get the default start time and `measure` is significantly optimized (25/30% faster) due to simplified parameter handling in JSI calls. * Before | (index) | Task name | Latency average (ns) | Latency median (ns) | Throughput average (ops/s) | Throughput median (ops/s) | Samples | | ------- | --------------------------------------------------------- | -------------------- | ------------------- | -------------------------- | ------------------------- | ------- | | 0 | 'mark (default)' | '1621.03 ± 1.17%' | '1562.00' | '636986 ± 0.01%' | '640205' | 616890 | | 1 | 'mark (with custom startTime)' | '1756.88 ± 1.15%' | '1693.00' | '586826 ± 0.01%' | '590667' | 569190 | | 2 | 'measure (default)' | '2424.66 ± 1.35%' | '2333.00' | '426122 ± 0.02%' | '428633' | 412429 | | 3 | 'measure (with start and end timestamps)' | '2679.96 ± 1.23%' | '2574.00' | '385266 ± 0.02%' | '388500' | 373140 | | 4 | 'measure (with mark names)' | '2713.49 ± 0.50%' | '2644.00' | '375383 ± 0.02%' | '378215' | 368530 | | 5 | 'clearMarks' | '691.13 ± 0.07%' | '681.00' | '1467016 ± 0.01%' | '1468429' | 1446900 | | 6 | 'clearMeasures' | '706.00 ± 0.05%' | '691.00' | '1431489 ± 0.01%' | '1447178' | 1416435 | | 7 | 'mark + clearMarks' | '2083.21 ± 1.14%' | '2003.00' | '497974 ± 0.01%' | '499251' | 480028 | | 8 | 'measure + clearMeasures (with start and end timestamps)' | '3085.14 ± 0.88%' | '2974.00' | '334337 ± 0.02%' | '336247' | 324135 | | 9 | 'measure + clearMeasures (with mark names)' | '2949.45 ± 0.62%' | '2884.00' | '345335 ± 0.02%' | '346741' | 339046 | * After | (index) | Task name | Latency average (ns) | Latency median (ns) | Throughput average (ops/s) | Throughput median (ops/s) | Samples | | ------- | --------------------------------------------------------- | -------------------- | ------------------- | -------------------------- | ------------------------- | ------- | | 0 | 'mark (default)' | '1740.06 ± 1.01%' | '1692.00' | '587400 ± 0.01%' | '591017' | 574695 | | 1 | 'mark (with custom startTime)' | '1661.64 ± 1.16%' | '1612.00' | '617453 ± 0.01%' | '620347' | 601815 | | 2 | 'measure (default)' | '1808.71 ± 1.28%' | '1753.00' | '566516 ± 0.01%' | '570451' | 552882 | | 3 | 'measure (with start and end timestamps)' | '1869.21 ± 1.00%' | '1823.00' | '546571 ± 0.01%' | '548546' | 534987 | | 4 | 'measure (with mark names)' | '2016.40 ± 0.74%' | '1983.00' | '502987 ± 0.01%' | '504286' | 496075 | | 5 | 'clearMarks' | '682.18 ± 0.03%' | '671.00' | '1476364 ± 0.01%' | '1490313' | 1465899 | | 6 | 'clearMeasures' | '686.78 ± 0.03%' | '681.00' | '1467264 ± 0.01%' | '1468429' | 1456081 | | 7 | 'mark + clearMarks' | '2148.90 ± 1.28%' | '2073.00' | '480925 ± 0.01%' | '482393' | 465356 | | 8 | 'measure + clearMeasures (with start and end timestamps)' | '2277.26 ± 1.10%' | '2204.00' | '451016 ± 0.01%' | '453721' | 439125 | | 9 | 'measure + clearMeasures (with mark names)' | '2277.82 ± 0.51%' | '2243.00' | '443853 ± 0.01%' | '445831' | 439016 | Reviewed By: hoxyq Differential Revision: D78193072 fbshipit-source-id: 03b52927a1999f19a2baf0d02335a959525d7add
… the "Track:" prefix (facebook#52614) Summary: Pull Request resolved: facebook#52614 Changelog: [internal] We're adding first-class support for custom tracks, so we can remove the legacy mechanism to specify tracks with the "Tracks:" prefix in the event names. Reviewed By: sbuggay Differential Revision: D78340910 fbshipit-source-id: cbbadd519baf7bb50072cb97d8cd1ccc87a8a35c
…nd performance.measure for DevTools (facebook#52613) Summary: Pull Request resolved: facebook#52613 Changelog: [internal] This adds first-class support for the `detail` field in `performance.mark` and `performance.measure`. Now that we have access the JS entry in native, we can access the `detail` field to propagate it to DevTools (and use it to extract track names for Perfetto). In order to avoid the performance overhead of always having to extract the `detail` field from the entry, this is done lazily only if we're actively profiling with DevTools or Perfetto. Reviewed By: sbuggay Differential Revision: D78340911 fbshipit-source-id: 383dd1cb6fcc8a04be9e65038503986f196e23c9
Summary: There are some duplicated function calls in `PerformanceEntryReporter::reportMark()` . I know this is a micro optimization but I feel this way the code is cleaner. This is called through `performance.mark` so there is potentially a tiny little performance improvement here? ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [INTERNAL] [FIXED] - Removed redundant checks in `PerformanceEntryReporter::reportMark()` Pull Request resolved: facebook#52756 Test Plan: The existing testing infrastructure should cover these callsites i believe Reviewed By: rubennorte Differential Revision: D78731441 Pulled By: cortinico fbshipit-source-id: e0de12c3c6f55e12eb454ea4b7081f3d6003126c
…performance/timeline/tests/PerformanceEntryReporterTest.cpp (facebook#52780) Summary: Pull Request resolved: facebook#52780 Reviewed By: rshest Differential Revision: D78798597 fbshipit-source-id: 28cdd8b2fdd4c5d5889f527ee7574f87e18215a7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the native c++ performance API to the latest, this supports:
The commits are cherry picked from:
Going from: 8989bc2 to c97741d
However, i left out the iOS podspec changes (which enables prebuild)