objectbox / objectbox-dart

Flutter database for super-fast Dart object persistence
https://docs.objectbox.io/getting-started
Apache License 2.0
927 stars 115 forks source link

Benchmarks #371

Closed simc closed 2 years ago

simc commented 2 years ago

I already made a comment about this on Reddit some time ago but I feel like the new benchmarks you added to your repo and pub.dev page are even more misleading than the blog post before:

You are comparing two databases that run asynchronously with objectbox which runs most of its operations synchronously blocking the UI isolate.

Async operations are expensive* in Dart (both in Dart itself and when calling a native library) but worth it since you don't want to block your UI isolate. Hive could be multiple times faster if it only used synchronous operations. And there are sqlite packages with synchronous API.

Quote from your guide how to benchmark databases:

Generally speaking, benchmarks should compare databases of similar type and not mix approaches that are too different.

Aside from this obvious flaw, I feel like the benchmark gives people the wrong idea. Hive is not even competing against objectbox. The two packages could not be more different and I can even see people using both in the same app.

Edit: *async operations are not actually expensive but just take more time since they need to wait until the event loop can handle the result.

greenrobot commented 2 years ago

Sorry, we do not share the views you laid out here. We try make benchmarks as fair as possible. And we are well aware of async vs. sync topic and we also believe that Dart as a language will make it easier to interop with multiple isolates in the future - so this is likely becoming a minor concern. It's our decision which libraries we pick, and a main criteria is popularity so that's why Hive makes perfect sense for us. I think most people would either choose Hive or ObjectBox as people want to store objects locally. This issue may be a matter of agree to disagree, so I suggest to close it.

simc commented 2 years ago

Sure but it is not a "view" that blocking the UI isolate is bad. And I don't see how potential future changes to the language make the benchmark fair now.

At least you should use SQLite synchronously.

greenrobot commented 2 years ago

Ofc, doing too much on the UI thread/isolate is bad, and that was not the point.

If there's SQLite lib that is similar in popularity as sqflite, please make a PR, and we're happy to include it.

simc commented 2 years ago

Another quote

Be honest, if you learn your benchmarks may be skewed and update them. In the end, everyone wins by getting more meaningful results.

greenrobot commented 2 years ago

Not sure why you quote that now. Yes, that's our values and we stand by those.

So far, your feedback was not very actionable. Frankly, if you do not like the criteria by which we choose libs, there's nothing much we can do about it. As I have written before, if you want to do a PR that adds value, we'll be happy about it.

Well, how about this: let's have a video call like we had before. Maybe this is not the most satisfying format for this conversation?

simc commented 2 years ago

My main criticism was not your choice of libraries but the fact that you use synchronous code from the UI isolate in a way that is not sustainable in real apps. Probably 95% of your users use objectbox in Flutter and you can't block the UI isolate more than 1-2ms with native code since you also have to encode/decode the objects in the UI isolate and Flutter also needs some time to render.

I also don't like how Dart handles threads but we have to live with it and it should be handled correctly in benchmarks. Otherwise it is just misleading to users who see the benchmark and don't have insight into the details.

(I also didn't find a mention in your examples or docs about the topic but that is none of my business)

Unfortunately I have no time to fix the benchmarks but I also don't think it is my job to do so. You can just add one or two sentences to the results to make clear what you actually measure and what not. Especially since there are very popular SQLite ffi packages (drift can also do it @simolus3?)

Sure, let's have a video call if you like :)

simolus3 commented 2 years ago

I didn't take a deep look, but intuitively I agree that comparing synchronous with asynchronous packages isn't a fair comparison without mentioning it at all. A Future completing in 10ms but allowing other tasks to run for 9.8ms of the time can be preferable to a synchronous call taking just 2ms.

If anyone wants to add synchronous sqlite to the benchmark, the sqlite3 and sqlite3_flutter_libs packages could be interesting. But my limited understanding is that objectbox provides asynchronous APIs too, why not use them in the benchmark against other asynchronous libraries?

greenrobot commented 2 years ago

@leisim

Disclaimer: It's common knowledge that one should any avoid heavy lifting in a UI thread. This is nothing that has to be discussed here.

I think I agree to your basic sentiments. Still, for the people that might read this, I'd like to do some clarifications...

you can't block the UI isolate more than 1-2ms with native code [...]

Were do you take these numbers from? This at least an oversimplification and may give readers a wrong impression. The math is that, even when you want to do 60 fps animation, it's 16 ms. Inflating some objects and stuff is usually much less than a ms. And I hope that Flutter has a rendering pipeline that does the hard work in yet another thread (e.g. Android does that, it's pretty basic pattern). And don't get me started on user experience when you e.g. enter a screen or press a button... Even if you would take 100 ms (I not saying you should), it wouldn't be an issue perception wise.

I do not agree on your choice of words like "it's not my job to fix yours". Also, I do not even think that is actually broken. E.g. if a library of our choosing does not offer the best suited APIs for the job (i.e. synchronous functions), we just have to take the next best thing it offers. Also, the actual impact of this may be negligible depending on individual cases.

Again, I also sense some constructive content in your writing - maybe I'll extract actual TODOs into a new ticket to be clear about that.

greenrobot commented 2 years ago

I agree that comparing synchronous with asynchronous packages isn't a fair comparison without mentioning it at all

I think we all agree on that. :-)

PS.: As I wrote before, the absence of synchronous APIs may require using async ones. If that really makes a difference depends highly on individual cases. E.g. if you do one operation in the background taking 1s and async adds 0.1ms to that, nobody cares as the error is not significant.

simc commented 2 years ago

I agree that getting a few objects is no problem. But as soon as you start receiving many objects or even use sorting in queries that is no longer true.

This at least an oversimplification and may give readers a wrong impression. The math is that, even when you want to do 60 fps animation, it's 16 ms.

16ms for build, layout, painting, db access, other logic etc. 8ms for 120fps devices. Not all of these have to be executed for every frame but you can't predict when they run.

Inflating some objects and stuff is usually much less than a ms.

You can decode few objects in a millisecond. I'd need to do some benchmarks but my guess is that decoding 100 objects takes more than 1ms. Especially on older devices.

And I hope that Flutter has a rendering pipeline that does the hard work in yet another thread

Skia runs in a background thread but all the build, layouting, animations and painting is done in the UI isolate.

Even if you would take 100 ms (I not saying you should), it wouldn't be an issue perception wise.

I strongly disagree with that statement. As long as there are no animations it might be fine but as soon as you display a spinner or scroll a list it is noticeable for sure and bad user experience. And often, querying objects is only the first step since they also need to be shown in a list etc. Often the UI isolate is quite busy before / after the db read or write.

Also, I do not even think that is actually broken.

It is not broken but users that just see the benchmark will get the wrong idea for sure!

if a library of our choosing does not offer the best suited APIs for the job (i.e. synchronous functions)

I still fail to see how synchronous APIs are "best suited" for the UI isolate. "Unless you have a specific reason for using the synchronous version of a method, prefer the asynchronous version to avoid blocking your program." File docs (didn't find many more examples in the docs because there don't seem to be any synchronous Flutter APIs)

And you claim "handling millions of objects resource-efficiently with ease". I just don't see how that fits together.

if you do one operation in the background taking 1s and async adds 0.1ms to that, nobody cares as the error is not significant.

It makes a much bigger difference. My guess is that you can at least double or triple the SQLite performance (platform channels are ridiculous haha). Synchronous Hive might not be such a drastic improvement but still maybe 50%?

Either way, I'll stop discussing this issue. You choose how to do your benchmark. I was just a little surprised to see it being done this way :)

greenrobot commented 2 years ago

Yes, let's stop here. I'll try to not give in to the urge to comment on the latest batch of your writing. :upside_down_face:

Time is better spent on writing some awesome DBs, I think we can agree on that. :smirk: