search

rocicorp / replicache

Realtime Sync for Any Backend Stack
https://doc.replicache.dev
1.05k stars 37 forks source link

Test performance benefit of removing copies in read transactions #885

Closed aboodman closed 1 year ago

aboodman commented 2 years ago

Currently Replicache returns deep clones in all read interfaces for safety. This was introduced in rocicorp/replicache#479 and the rationale is there.

However, this must be really expensive, both in cpu time and gc pressure, and it seems like it would be hard to see in the profile because the gc's will tend to be decoupled from cause.

I would like to test out the idea of skipping the clone for read transactions and see what it does for our benchmarks. If we ship this, then we would also want to do rocicorp/mono#136 to prevent typescript users from modifying this data.

arv commented 2 years ago

Currently Replicache returns deep clones in all read interfaces for safety. This was introduced in rocicorp/replicache#479 and the rationale is there.

We do not do that. ReadTransaction get returns a ReadonlyJSONValue.

We only clone for the WriteTransaction which returns a JSONValue.

aboodman commented 2 years ago

Oh. I see. I misread the code. 😳. I wonder what @tmcw was seeing then with object identities changing. That seems like it would happen only when a key/value pair changes, which is expected.

arv commented 2 years ago

We clone in put so what you put in is not going to be the same object as what you get out.

I still think there is room for improvement even if we do clone...

But I would be happier if we got rid of all the clones and told people not to do that or things will break!

arv commented 2 years ago

Another option is to conditionally clone/freeze. React uses process.env.NODE_ENV as a signal for doing expensive correctness checks. We could do the same.

arv commented 2 years ago

The current plan is to deep freeze in debug mode and do nothing in release mode (with the risk of things not working if people only test in release mode).