search

facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
118.96k stars 24.31k forks source link

Improve performance of `performance.clearMarks` #45122

Closed rubennorte closed 1 month ago

rubennorte commented 4 months ago

Description

We have a gated implementation of a new Performance API (via the global performance object) and PerformanceObserver.

In this implementation (see PerformanceEntryReporter), all performance entries are stored in fixed size circular buffers, which has several problems:

  1. All the entry types have the same buffer size. This is against the spec, as buffer sizes depend on the type of entry (see https://w3c.github.io/timing-entrytypes-registry/#registry).
  2. Performance for some operations isn't great. Specifically performance.clearMarks is currently an O(n) operation when given a specific entry name.

The second problem is especially relevant in a pattern commonly used to mark points/measures for profiling in the Performance tab without keeping the data in memory for performance observer:

performance.mark('point');
performance.clearMark('point');

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.

We should refactor the implementation to:

  1. Explicitly separate the buffer array into specific entries for specific types.
  2. Replace the use of the circular buffer with a unordered_map mapping entry name to entry for marks and measures. We should keep the circular buffer for event types, and reduce its size to 150 (as per the spec).

React Native Version

0.74.2 / main

Affected Platforms

Runtime - Android, Runtime - iOS, Runtime - Web, Runtime - Desktop

Areas

Bridgeless - The New Initialization Flow, Other (please specify)

Web Convergence

rubennorte commented 4 months ago

cc @robik :)

robik commented 4 months ago

@rubennorte Should the refactor keep the API compatibility?

rubennorte commented 4 months ago

@rubennorte Should the refactor keep the API compatibility?

Yes, that's would be preferred for now.

I think we could use a better API but for the sake of keeping the changes scoped we should address this first.

robik commented 4 months ago

@rubennorte I managed to refactor this, but not sure how to enable it so I can test it properly 😅 Would simply adding the turbomodule be sufficient?

rubennorte commented 4 months ago

@rubennorte I managed to refactor this, but not sure how to enable it so I can test it properly 😅 Would simply adding the turbomodule be sufficient?

Yeah, that should suffice. You can include it in react-native/ReactCommon/react/nativemodule/defaults/DefaultTurboModules.cpp and test the API in RN Tester (Performance API Example).