Feature: Benchmark Onyx #70
Conversation
process.env.BENCHMARK_ONYX is not available in react-native out of the box
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
Metering xlsx sample: https://docs.google.com/spreadsheets/d/1R4YPeG6rHbTiY3gcKINV-kLFsW37ITnfhV_MYnWmJzs/edit?usp=sharing
|
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
Now that I look at the data if (mergeQueue[key]) {
mergeQueue[key].push(val);
return Promise.resolve();
}It returns early when it's filling a queue |
lib/Onyx.js
Outdated
| @@ -324,15 +314,15 @@ function connect(mapping) { | |||
| // React components are an exception since we'll want to send their | |||
| // initial data as a single object when using collection keys. | |||
| if (mapping.withOnyxInstance && isCollectionKey(mapping.key)) { | |||
| Promise.all(_.map(matchingKeys, key => get(key))) | |||
| Promise.all(_.map(matchingKeys, key => OnyxInternal.get(key))) | |||
There was a problem hiding this comment.
When this was just get we have no way to replace dynamically replace it with a decorated get that would collect statistics.
By providing get from an object - OnyxInternal.get - we can always change the method on the object to alter the implementation of get - in our case we decorate it with stats collection logic
Regarding this comment. The only calls to Make A simpler alternative would be to just manually wrap all the promises we want to measure and tag them. But you'd have to unwind the decorator pattern stuff. |
Sounds like this could be another reason why benchmarking inline with a higher order function that wraps any |
kidroca
force-pushed
the
kidroca/benchmark
branch
from
efb1e1e to
eb7b109
Compare
kidroca
force-pushed
the
kidroca/benchmark
branch
from
36ee6a4 to
6275a02
Compare
kidroca
force-pushed
the
kidroca/benchmark
branch
from
6275a02 to
74809be
Compare
|
Updated Addressed all the points in @marcaaron's comment This is an example output: Total time is lot React.Native.Debugger.-.Google.Chrome.2021-05-22.00-56-52.mp4
I didn't run it through the console, I've added some code to E.cash to print statistics each time navigation state changed: NavigationRoot.js parseAndStoreRoute(state) {
if (!state) {
return;
}
const path = getPathFromState(state, linkingConfig.config);
setCurrentURL(path);
// Added this (previously it was a method on this component)
if (Onyx.printMetrics) {
Onyx.printMetrics();
Onyx.resetMetrics();
}
}@marcaaron I've removed |
|
Updated Resolved merge conflicts |
Yes I've just added a fix: Because the original non decorated methods are instantly added to const Onyx = {
connect,
disconnect,
set,
multiSet,
merge,
mergeCollection,
clear,
init,
registerLogger,
addToEvictionBlockList,
removeFromEvictionBlockList,
isSafeEvictionKey,
};And this happens before We need to re-expose them like so In applyDecorators() // Re-expose decorated methods
Onyx.set = set;
Onyx.multiSet = multiSet;
Onyx.clear = clear;
Onyx.merge = merge;
Onyx.mergeCollection = mergeCollection; |
|
Updated with requested changes |
| /* | ||
| * Then handlers added here are not affecting the original promise | ||
| * They crate a separate chain that's not exposed (returned) to the original caller | ||
| * */ |
There was a problem hiding this comment.
| /* | |
| * Then handlers added here are not affecting the original promise | |
| * They crate a separate chain that's not exposed (returned) to the original caller | |
| * */ | |
| // The handlers added here are not affecting the original promise | |
| // They create a separate chain that's not exposed (returned) to the original caller |
@marcaaron
Details
Moved some methods to
Onyx.internal.js- e.g. to test private methods likegetthrough the decorator patternCreated
decorateWithMetricslib that serves to decorate Onyx methods similarly to how HOCs are used to enhance componentsUpdated some methods so they return the promises they work with so that they can be metered
Added
captureMetricsflag to Onyx.init. Initially the intent was to useprocess.env.BENCHMARK_ONYXbutprocess.envhas onlyNODE_ENVso far - this can be configured after the draft is finalizedCreated a sample usage that will print statistics for each navigation state change (only in Chrome dev tools - debug mode). Don't forget to set the
BENCHMARK_ONYX=truein.env: https://github.com/kidroca/Expensify.cash/tree/kidroca/sample-onyx-benchmarkSome methods cannot be tested as easy like the
connectmethod, they would need some changes to incorporate the decorator - e.g. don't measure the wholeconnectbut move the promise based logic to a separate method and measure that method - this way connect can still return aconnectionIDor aremoveListenercallbackRelated Issues
#65
Expensify/App#2762
Automated Tests
Added basic covering tests TBD
Linked PRs