perf: optimize reactive state handlers #69

nolanlawson · 2021-10-29T17:24:38Z

This is a performance optimization for two reasons:

We avoid creating an extra object ({ readOnly: ..., reactive: ... }). Instead, we just keep track of the readOnly and reactive functions separately.
We avoid calling Object.defineProperty when the getter is called (since we don't have an object, it's no longer needed).

In the perf benchmarks, this is a 1-7% improvement and no regression:

This benchmark uses this PR on top of v1.0.1 (to avoid including changes in v1.1.3), with Tachometer set to 1% horizon, 20 min timeout, and 100 min samples, on a MacBook Pro running Chrome 95.

nolanlawson · 2021-10-29T17:27:06Z

src/reactive-membrane.ts

@@ -72,7 +66,8 @@ export class ReactiveMembrane {
    valueObserved: ReactiveMembraneAccessCallback = defaultValueObserved;
    valueIsObservable: ReactiveMembraneObservableCallback = defaultValueIsObservable;
    tagPropertyKey: ProxyPropertyKey | undefined;
-    private objectGraph: WeakMap<any, ReactiveState> = new WeakMap();
+    private readOnlyObjectGraph: WeakMap<any, any> = new WeakMap();
+    private reactiveObjectGraph: WeakMap<any, any> = new WeakMap();


Having an extra object here doesn't seem to be a big deal. We only ever create one ReactiveMembrane object, so this amounts to one extra WeakMap.

nolanlawson · 2021-10-29T17:29:52Z

src/reactive-membrane.ts

            // when trying to extract the writable version of a readonly
            // we return the readonly.
-            return o.readOnly === value ? value : o.reactive;
+            return readOnly === value ? value : this.getReactiveHandler(unwrappedValue, distorted, false);


I was a bit worried that this might actually be a perf regression (due to calling WeakMap.get() twice instead of once), but in our perf benchmarks at least, it doesn't seem to have a negative impact. I'm guessing this code path isn't hit very frequently.

Yeah, this code path is only hit for @track fields, and we do not have any perf test with @track.

@nolanlawson This makes me wonder if we could remove the creation of the readOnly proxy in this routine? my mental model is: this check is in place for when we want to getProxy of a readonly proxy, therefore it should already be in the readOnlyObjectGraph, and if the value is not in the readOnlyObjectGraph, then we should create a this.getReactiveHandler(unwrappedValue, distorted, false).

Even if is not visible in the perf tests (because all of their values are @api/readonly), for cases in which the values are set to a writable proxy (ex: data from lds) or @track foo = {...}, it will be noticeable, since we won't create a read-only proxy when we shouldn't (saves mem + processing time)

Thanks for the details! I'll add a benchmark for @track. I think it's important to test this use case.

As for not creating the extra readOnly, your explanation makes sense to me. We should be able to validate it with a @track benchmark.

@jodarove Made the change you recommended, and added a couple tests for safety. I'll re-run the perf tests with @track included.

jodarove

With this change, we can remove the (unnecessary) creation of the readOnly handler in the getProxy routine by checking if the distorted value is in the readOnlyObjectGraph and matches value. When it is not, we should return the writable proxy.

nolanlawson · 2021-10-29T21:25:49Z

@jodarove I re-ran the benchmark with the change you recommended, using a slight tweak to the perf tests to cover @track. Looks like it's an even bigger perf improvement than before!

I wouldn't worry about that 1% regression; it's only a 0.4ms difference so probably just noise.

nolanlawson · 2021-10-29T23:06:28Z

Ran on this new benchmark (salesforce/lwc#2560), it also shows an improvement there (10%):

jodarove

this is great!

caridy · 2021-11-01T17:43:48Z

src/reactive-membrane.ts

-        let reactiveState = objectGraph.get(distortedValue);
-        if (reactiveState) {
-            return reactiveState;
+    private getReactiveHandler(value: any, distortedValue: any, readOnly: boolean): any {


avoid boolean arguments, instead just pass the corresponding map, that should also make the code more readable.

or make it two methods

I think I rather prefer two methods.

I can make it two methods. It just requires duplicating ~5 lines of code, which is no big deal.

caridy

LGTM

ravijayaramappa · 2021-11-02T21:30:51Z

src/reactive-membrane.ts

+            const handler = new ReadOnlyHandler(this, distortedValue);
+            proxy = new Proxy(createShadowTarget(distortedValue), handler);
+            registerProxy(proxy, value);
+            this.readOnlyObjectGraph.set(distortedValue, proxy);


~~Any specific reason to use the distortedValue as the key instead of the value?~~

~~IIRC, the original implementation does not have an invariant that two values cannot produce the same distorted value.~~
nvm, I see the original implementation also maintained a map of distorted value -> ReactiveState. So you are not introducing any new behavior.

@ravijayaramappa Correct, yeah, it's the same as before.

I'm not really sure how the distortion works, though. In LWC, we don't do any distortion, and the tests in this repo don't do a lot of distortion either. Arguably we could release a breaking change for this library to just get rid of the distortion entirely and use the raw value everywhere (assuming there aren't any major consumers of this library that are relying on the distortion).

I'm ok with removing distortions.

nolanlawson requested review from caridy, jodarove, ravijayaramappa and divmain October 29, 2021 17:24

nolanlawson commented Oct 29, 2021

View reviewed changes

jodarove requested changes Oct 29, 2021

View reviewed changes

nolanlawson added 2 commits October 29, 2021 13:24

perf: optimize reactive state handlers

9c046e6

perf: avoid creating readonly handler unnecessarily

d64791f

nolanlawson force-pushed the nolan/reactive-state branch from f692aa1 to d64791f Compare October 29, 2021 20:26

nolanlawson requested a review from jodarove October 29, 2021 21:28

nolanlawson added 3 commits October 29, 2021 14:45

fix: add missing semicolon

2def2b6

fix: add another missing semicolon

05de794

fix: add yet another missing semicolon

67642ed

nolanlawson mentioned this pull request Oct 29, 2021

perf: add benchmark for @track salesforce/lwc#2560

Merged

jodarove approved these changes Nov 1, 2021

View reviewed changes

caridy reviewed Nov 1, 2021

View reviewed changes

fix: duplicate the methods

f13761d

jodarove approved these changes Nov 1, 2021

View reviewed changes

nolanlawson requested a review from caridy November 1, 2021 20:16

caridy approved these changes Nov 1, 2021

View reviewed changes

nolanlawson merged commit cb575cd into master Nov 1, 2021

nolanlawson mentioned this pull request Nov 1, 2021

chore: update observable-membrane to 1.1.5 salesforce/lwc#2561

Merged

ravijayaramappa deleted the nolan/reactive-state branch November 2, 2021 21:12

ravijayaramappa reviewed Nov 2, 2021

View reviewed changes

nolanlawson mentioned this pull request Nov 3, 2021

Remove distortions #74

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: optimize reactive state handlers #69

perf: optimize reactive state handlers #69

nolanlawson commented Oct 29, 2021

nolanlawson Oct 29, 2021

nolanlawson Oct 29, 2021

jodarove Oct 29, 2021 •

edited

nolanlawson Oct 29, 2021

nolanlawson Oct 29, 2021

jodarove left a comment •

edited

nolanlawson commented Oct 29, 2021

nolanlawson commented Oct 29, 2021

jodarove left a comment

caridy Nov 1, 2021

caridy Nov 1, 2021

caridy Nov 1, 2021

nolanlawson Nov 1, 2021

caridy left a comment

ravijayaramappa Nov 2, 2021 •

edited

nolanlawson Nov 2, 2021

caridy Nov 2, 2021

perf: optimize reactive state handlers #69

perf: optimize reactive state handlers #69

Conversation

nolanlawson commented Oct 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodarove Oct 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodarove left a comment • edited

Choose a reason for hiding this comment

nolanlawson commented Oct 29, 2021

nolanlawson commented Oct 29, 2021

jodarove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

ravijayaramappa Nov 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodarove Oct 29, 2021 •

edited

jodarove left a comment •

edited

ravijayaramappa Nov 2, 2021 •

edited