Experiment with no proxy for objects #1031

ericanderson · 2024-12-03T17:47:50Z

BLUF

This PR removes the proxy objects in favor of simply adding functionality to objects from the wire. Originally this was to make it easier for people when they console.log() objects but additionally it seems we get a per object size reduction from 2,324 bytes per object in this test bed to 1,332 bytes. This does increase construction of the objects by 13% which seems reasonable.

What does it cost us?

We lose a few things in this exchange:

The ability to do $updateInternalValues() and have it reflect across any object that was $as()'d. OSDK objects effectively become immutable in this scenario. I think this is okay, it just removes some future ideas I had.
An obvious tie in for telemetry metrics on per property usage. Before, in "telemetry mode" we would have been able to just hook into the proxy property access. We could still build this but will need to rethink how it would work.

What else did we gain?

I think the code is a bit easier to reason about after we remove the proxies. Also the bundled minified client for nodejs (via esbuild) is reduced by about 4k in bytes.

Other notes

In a separate repo where I originally experimented with this, retrieving properties from a proxy vs directly on an object was about 2x as expensive. So for anyone iterating over all of the objects returned, the 13% construction time increase would be saved on property access.

Detailed benchmark info

On my machine, createObjects.ts benchmark stats prior (1b60b3d):

      "stats": {
        "time": {
          "std": 20.022638911991578,
          "mean": 542.9110000000001,
          "min": 523,
          "max": 593.92
        },
        "heapUsed": {
          "std": 44488.05771620065,
          "mean": 232406548.8,
          "min": 232355784,
          "max": 232456888
        },
        "rss": {
          "std": 2388346.6567709134,
          "mean": 237371392,
          "min": 233029632,
          "max": 241123328
        }
      }

So per object in the test data we were paying about 232406548.8/1000/100 ~= 2,324 bytes per object.

On my machine, createObjects.ts benchmark stats after:

      "stats": {
        "time": {
          "std": 19.36567313573169,
          "mean": 613.712,
          "min": 589.16,
          "max": 653.87
        },
        "heapUsed": {
          "std": 21950.770067585327,
          "mean": 133270992.8,
          "min": 133241184,
          "max": 133293728
        },
        "rss": {
          "std": 2037190.144079202,
          "mean": 158664294.4,
          "min": 155582464,
          "max": 162185216
        }
      }

133270992.8/1000/100 ~= 1,332 bytes per object.

ericanderson · 2024-12-03T17:48:43Z

packages/client/src/fetchMetadata.test.ts

+            "multiplicity": false,
+            "nullable": true,
+            "type": "string",
+          },


We were assuming this property in places but did not actually define it which caused me some problems early on.

ericanderson · 2024-12-03T17:49:44Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkInterface.ts

-    [InterfaceDefRef]: interfaceDef,
-  };
+  const interfaceHolder: InterfaceHolderOwnProps<Q> = Object.create(null, {
+    [UnderlyingOsdkObject]: { value: underlying },


I don't want these to be enumerable.

ericanderson · 2024-12-03T17:50:24Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkInterface.ts

        case "$primaryKey":
        case "$title":
        case "$objectType":
        case "$rid":
-          return underlying[p] != null
-            ? Reflect.getOwnPropertyDescriptor(underlying, p)


Can't reflect in this version because the underlying is now frozen.

ericanderson · 2024-12-03T17:51:04Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkInterface.ts

@@ -123,6 +131,8 @@ function createInterfaceProxyHandler(
        "$primaryKey",
        ...(underlying["$rid"] ? ["$rid"] : []),
        "$title",
+        UnderlyingOsdkObject,
+        InterfaceDefRef,


This is required now that they are no longer configurable/enumerable.

Hmm, I'm still kinda confused why we add these here

ericanderson · 2024-12-03T18:02:23Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkObject.ts

+  {
+    {
+      {
+        {


These just keep the diff simple. We can remove them before merge or in follow up

ssanjay1 · 2024-12-04T13:17:10Z

packages/client/src/object/convertWireToOsdkObjects/ObjectHolder.ts

@@ -34,12 +33,10 @@ export interface ObjectHolderPrototypeOwnProps {
  readonly [ClientRef]: MinimalClient;
  readonly "$as": DollarAsFn;
  readonly "$link": ReturnType<typeof get$link>;
-  readonly "$updateInternalValues": (newValues: Record<string, any>) => void;


Ok so now that we're just using the raw object, we don't need this

Yup, this is why I said we have to give up on this functionality to achieve this.

ssanjay1 · 2024-12-04T13:25:22Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkInterface.ts

@@ -123,6 +131,8 @@ function createInterfaceProxyHandler(
        "$primaryKey",
        ...(underlying["$rid"] ? ["$rid"] : []),
        "$title",
+        UnderlyingOsdkObject,
+        InterfaceDefRef,


Hmm, I'm still kinda confused why we add these here

ssanjay1 · 2024-12-04T13:31:03Z

packages/client/src/object/convertWireToOsdkObjects/createOsdkObject.ts

+  Object.defineProperties(rawObj, {
+    [ObjectDefRef]: { value: objectDef, enumerable: false },
+    [ClientRef]: { value: client, enumerable: false },
+    [RawObject]: {


What's the difference between RawObject and UnderlyingOsdkObject? I think I'm getting tripped up because I know type wise the RawObject is what we get over the wire and the underlying is like OsdkBase, but here we set both values to rawObj? Probably missing something here

Nothing. But Im not trying to change ALL the code in the system to demonstrate JUST these objects being raw instead of proxies. The existing contract required both of these things, they just happen to be the same now.

If we go forward, this wouldn't be kept like this, we would clean it all up. But redoing the contract for interfaces would just muddy up this change (and I might as well just remove all proxies at that point since it would be far reaching).

The goal here was to show that the objects could not be proxies and what that would be like (roughly) so we could discuss taking the next steps.

reorder for diff reorder for diff reorder for diff reorder for diff update snapshot

ericanderson commented Dec 3, 2024

View reviewed changes

ssanjay1 reviewed Dec 4, 2024

View reviewed changes

ericanderson force-pushed the ea.experiment-with-no-proxy branch 2 times, most recently from 9a7b268 to d496d03 Compare December 5, 2024 13:58

ericanderson added 4 commits December 6, 2024 12:58

Experiment with no proxy for objects

df14c2c

reorder for diff reorder for diff reorder for diff reorder for diff update snapshot

Remove proxy for interface

3ffd370

Cleanup getDolloarLink

75db812

Changeset

8b022a2

ericanderson force-pushed the ea.experiment-with-no-proxy branch from ff79245 to 8b022a2 Compare December 6, 2024 17:59

nihalbhatnagar approved these changes Dec 9, 2024

View reviewed changes

ericanderson merged commit 720218d into main Dec 9, 2024
8 checks passed

ericanderson deleted the ea.experiment-with-no-proxy branch December 9, 2024 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment with no proxy for objects #1031

Experiment with no proxy for objects #1031

ericanderson commented Dec 3, 2024 •

edited

Loading

ericanderson Dec 3, 2024

ericanderson Dec 3, 2024

ericanderson Dec 3, 2024

ericanderson Dec 3, 2024

ssanjay1 Dec 4, 2024

ericanderson Dec 3, 2024

ssanjay1 Dec 4, 2024

ericanderson Dec 4, 2024

ssanjay1 Dec 4, 2024

ssanjay1 Dec 4, 2024

ericanderson Dec 4, 2024

Experiment with no proxy for objects #1031

Experiment with no proxy for objects #1031

Conversation

ericanderson commented Dec 3, 2024 • edited Loading

BLUF

What does it cost us?

What else did we gain?

Other notes

Detailed benchmark info

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

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

Choose a reason for hiding this comment

ericanderson commented Dec 3, 2024 •

edited

Loading