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

New implementation of PerformanceObserver and related APIs (fixes #45122) #45206

Closed
wants to merge 60 commits into from
Closed

New implementation of PerformanceObserver and related APIs (fixes #45122) #45206

wants to merge 60 commits into from

Conversation

robik
Copy link
Contributor

@robik robik commented Jun 27, 2024 •
edited
Loading

Summary:

Fix #45122.

performance.mark is currently O(1) but performance.clearMark is O(n) (being n the number of entries in the buffer), which makes this operation very slow.

Changes overview

  • Created new PerformanceEntryBuffer abstraction with the following subtypes, that differ on how entries are stored:
    • PerformanceEntryCircularBuffer - stores them in a ring buffer that was already implemented, removed key lookup cache (BoundedConsumableBuffer)
    • PerformanceEntryKeyedBuffer - stores them in a unordered_map, allowing for faster retrieval by type
    • PerformanceEntryLinearBuffer - a simple infinite buffer based on std::vector, currently used in a PerformanceObserver
  • Created PerformanceObserver abstraction on native side.
  • Created PerformanceObserverRegistry that collects active observers and forwards entries to observers that should retrieve them
  • Moved some method implementations to .cpp files to reduce potential compilation time slowdown. As the PerformanceEntryReporter can be included from anywhere in the code, it will be beneficial to make header files as light as possible.
  • Add comments to methods that note which standard is the method from/for.
  • Added some [[nodiscard]] attributes
  • Since the logic of routing entries to observers is moved to native side, JS side of the code got much simplified
  • If ever needed, PerformanceObserver can be created from native-side

Standards covered:

Changelog:

[GENERAL] [CHANGED] - Refactored performance-timeline module, which should improve performance of performance.clearMarks and performance.clearMeasures

Test Plan:

n/a

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 27, 2024
@robik robik force-pushed the robik/improve-performance-observer branch from 2ed73a9 to 5de7d48 Compare July 10, 2024 11:09
@robik
Copy link
Contributor Author

robik commented Jul 10, 2024

Pushed early changes for approach review. Still working on updating TurboModule and its spec to match new API.

@robik robik force-pushed the robik/improve-performance-observer branch from 5de7d48 to 7b45779 Compare July 24, 2024 14:11
@robik robik marked this pull request as ready for review July 24, 2024 14:11
@robik
Copy link
Contributor Author

robik commented Jul 24, 2024

Ready for an early review. Will undo changes related to exposing Performance TurboModule and still needs some testing from my side.

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 24, 2024
rubennorte
rubennorte requested changes Jul 31, 2024
View reviewed changes
Copy link
Contributor

@rubennorte rubennorte left a comment

Choose a reason for hiding this comment

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

This is a larger refactor than I was expecting, but I like the direction we're taking here. I made an initial pass on the PR (please forgive the amount of comments, this is very large). Please let me know what you think, and thanks so much for this!

packages/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformance.cpp Outdated Show resolved Hide resolved
...ges/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.cpp Show resolved Hide resolved
packages/react-native/ReactCommon/react/nativemodule/defaults/DefaultTurboModules.cpp Outdated
@@ -28,6 +30,14 @@ namespace facebook::react {
return std::make_shared<NativeIdleCallbacks>(jsInvoker);
}

if (name == NativePerformance::kModuleName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to enable this by default yet, until we've had a chance to test it thoroughly at Meta first. Please revert this for the PR (I understand you still need it to test it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the comment about this being temporary as well :P

...ges/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.cpp Show resolved Hide resolved
...ges/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.h Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserverRegistry.h Outdated Show resolved Hide resolved
@robik robik force-pushed the robik/improve-performance-observer branch 2 times, most recently from 844f334 to 8a77bcd Compare August 1, 2024 10:10
@robik
Copy link
Contributor Author

robik commented Aug 8, 2024

@rubennorte I've allowed myself to resolve comments that I think are corrected :) The remaining ones I still need some input.
Sorry for large batch of changes again, but I think I simplified a lot of things.

FYI: still not fully tested on my side 😅

rubennorte
rubennorte requested changes Sep 2, 2024
View reviewed changes
packages/react-native/ReactCommon/react/nativemodule/defaults/DefaultTurboModules.cpp Outdated
@@ -28,6 +30,14 @@ namespace facebook::react {
return std::make_shared<NativeIdleCallbacks>(jsInvoker);
}

if (name == NativePerformance::kModuleName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the comment about this being temporary as well :P

...ages/react-native/ReactCommon/react/nativemodule/defaults/React-defaultsnativemodule.podspec Outdated Show resolved Hide resolved
...ges/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.cpp Outdated Show resolved Hide resolved
...ges/react-native/ReactCommon/react/nativemodule/webperformance/NativePerformanceObserver.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/BoundedConsumableBuffer.h Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.cpp Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.h Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.h Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.h Outdated Show resolved Hide resolved
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserverRegistry.h Outdated Show resolved Hide resolved
@robik robik force-pushed the robik/improve-performance-observer branch from caf3b65 to 8772490 Compare September 13, 2024 11:26
@robik robik force-pushed the robik/improve-performance-observer branch from e57a510 to 8fe46c2 Compare September 20, 2024 09:18
@facebook-github-bot
Copy link
Contributor

@rubennorte has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@rubennorte rubennorte changed the title refactor: use map for user timing API for better performance (fixes #45122) New implementation of PerformanceObserver and related APIs (fixes #45122) Sep 20, 2024
@facebook-github-bot
Copy link
Contributor

@rubennorte has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

rubennorte
rubennorte reviewed Sep 23, 2024
View reviewed changes
packages/react-native/ReactCommon/react/performance/timeline/PerformanceObserver.cpp
}

if (observedTypes_.contains(entry.entryType)) {
buffer_.push_back(entry);
Copy link
Contributor

Choose a reason for hiding this comment

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

oops, we're not scheduling the task to notify the observer anywhere :S

Copy link
Contributor Author

@robik robik Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I need to get those unittests running locally somehow 🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at it again, we call the callback below, which is passed from NativePerformanceObserver to schedule a task 🤔

@facebook-github-bot
Copy link
Contributor

@rubennorte merged this pull request in 38b3b21.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 26, 2024
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @robik in 38b3b21

When will my fix make it into a release? | How to file a pick request?

This was referenced Oct 7, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rubennorte rubennorte rubennorte requested changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve performance of performance.clearMarks
4 participants
@robik @facebook-github-bot @react-native-bot @rubennorte