supabase / postgrest-dart

Dart client for PostgREST
https://supabase.com
MIT License
136 stars 38 forks source link

feat: reuse isolate #90

Closed Vinzent03 closed 1 year ago

Vinzent03 commented 2 years ago

close https://github.com/supabase-community/supabase-flutter/issues/229 I'm creating an isolate in PostgrestClient and reuse that for json decoding. I'm not using it for encoding, because the size of the request body is hard to determine. (We could use the length of the toString(). What do you think?) I'm only using isolate for responses with size >10000 bytes. It's from this article, but I don't think he reuses isolates. We need testing to determine the best size.

I'm not using it for errors since I don't expect such huge error responses.

bdlukaa commented 2 years ago

I believe it's better we wait for https://github.com/supabase-community/supabase-flutter/issues/233 to be addressed before we do any changes to the current computation method.

dshukertjr commented 2 years ago

@Vinzent03 It's so amazing how quickly you can spin up these isolates related codes! I think this is an amazing starting point, but I have one question. When we call .from() from supabase-dart library, we initialize a new SupabaseQueryBuilder, but would we be able to reuse the same isolate every time we call .from()?

@bdlukaa I agree! It would be amazing if we had a standardized way of measuring the performance difference especially before and after this update. @Vinzent03 Let's work on this one first, and revisit this PR once we have nice performance metrics!

Vinzent03 commented 2 years ago

@dshukertjr Oh I see we don't create/use a PostgrestClient in SupabaseDart.from(). Let's do the performance test on postgrest-dart first and try to integrate it with supabase-dart afterwards.

dshukertjr commented 1 year ago

@Vinzent03 It seems like quite a few people are annoyed with debug mode being slow. Let's take care of creating bench marks later on, and try to ship this one! Did we have any blockers at the moment?

Vinzent03 commented 1 year ago

When using the package on a server and want to act like the user, they have to create a new supabase-dart instance every time, because we removed setAuth(), right? This would create a new isolate each time. I think we should add an option to disable isolates. Because we can't keep the isolate, we can't really take advantage of the isolate on server. For use in supabase-dart, we somehow have to make use of the PostgrestClient. Currently we only use it for rpc, but not for from. Maybe add stream per extension?

dshukertjr commented 1 year ago

When using the package on a server and want to act like the user, they have to create a new supabase-dart instance every time, because we removed setAuth(), right? This would create a new isolate each time. I think we should add an option to disable isolates. Because we can't keep the isolate, we can't really take advantage of the isolate on server.

Would you educate me if you know? Do they ever run code on Dart virtual machine for server side Dart code? The isolate performance issue was only noticeable on debug mode for Flutter apps, and was fine on release mode, so am wondering if we need to worry about the performance issue on server's end.

Vinzent03 commented 1 year ago

Would you educate me if you know? Do they ever run code on Dart virtual machine for server side Dart code?

Sorry what exactly do you mean?

Maybe it want noticeable in release, but isolate creation still takes a relatively long time I think. So keeping it, would be even better. By the way do isolates even make sense on a server? I do it know how these server packages work.

dshukertjr commented 1 year ago

Sorry what exactly do you mean?

Sorry for the confusion. I just didn't know if server side Dart uses Dart VM and wanted to ask you if you knew, but as you have said, even in release mode spinning up isolates so many time might build up to be an issue, so it might be good to still optimizing them as well.

By the way do isolates even make sense on a server?

Great point. I don't think it has as big of an effect as compared to Flutter apps, but I think multi-threading still has some benefits.

Vinzent03 commented 1 year ago

@dshukertjr Sorry, I don't know how Dart is executed on a server.

Everything should be fine now, right?

Vinzent03 commented 1 year ago

One more thing, do we want an instance of PostgresIsolate to be reinitializable after dispose, or can you call init only one time? We should add an assertion in this case.

Additionally we may rename init to initialize.

dshukertjr commented 1 year ago

@Vinzent03

One more thing, do we want an instance of PostgresIsolate to be reinitializable after dispose, or can you call init only one time? We should add an assertion in this case.

I think disallowing it to be initialized after disposing it might make more sense in general. I haven't seen too many things that allows re-initialization after dispose in general, aside from Supabase.initialize.

Additionally we may rename init to initialize.

I'm a fan of initialize than just init!

Also am wondering how we can use the same code for functions-dart isolate. Would it be better if we just publish a separate Dart package to take care of the json serialization using isolates so that we can reuse the code and have a common class?

Vinzent03 commented 1 year ago

@dshukertjr It could be useful to use one isolate for postgrest and functions. Publishing another package may be the only way. Do you have a name in mind? The capabilities are independent of supabase. Something with json encoding/decoding and isolate.

Vinzent03 commented 1 year ago

Or instead of reinventing the wheel, we could use existing packages like worker_manager as mentioned in this cookbook. But it might not be a good idea to use this package in our package, because we would use a singleton, which may interferes with the outer app.

dshukertjr commented 1 year ago

@Vinzent03 I did a quick search of packages out there, but unfortunately couldn't find a great one. worker_manager does seem great, but as you have mentioned, the singleton might be an issue as you said. I don't want to take away the opportunity to name it, since you pretty much single handedly created it!

Vinzent03 commented 1 year ago

Do you want to publish it under supabase on pub.dev or do you want me to publish it? I will think about a name.

dshukertjr commented 1 year ago

@Vinzent03 That too can be up to you! More than happy to let you publish it under your pub.dev account, or under supabase, whichever you prefer!

Vinzent03 commented 1 year ago

Thank you. Always happy to improve the package :+1: