concurrent document firestore fetches is much slower than firebase-admin-node

[goldensunliu]

Describe your environment

• Operating System version: OS X El Capitan. v 10.11.6
• Firebase SDK version: 4.5.1
• Firebase Product: firestore

Describe the problem

When fetching about 100 documents by their ids using the firebase-js-sdk, I have noticed a significant delay till all fetches have been resolved. between 1 ~ 2 seconds.

On the other hand, the same fetch done using firebase-admin-node has been consistently outperforming the firebase-js-sdk at about 200 ~ 300ms

Steps to reproduce:

Use the sdk to create multiple docs fetch promises and measure how long till all of them are resolved.

And then compare the time took with the same code executed using a firestore from the firebase-admin-node

Relevant Code:

Here is the code that is being executed by both the firebase-js-sdk and the firebase-admin-node

const petIds = // Ids of pet docs I wanna fetch
const time = new Date().getTime()
// create the fetches
const fetches = petIds.map(id => {
    return firestore.collection('pets').doc(id).get()
})
// wait for them to resolve
const docs = await Promise.all(fetches)
console.log(`It took ${new Date().getTime() - time} ms`)

[ihateids]

Hello,

I have the same experience and I see others complaining about firestore performance too, I believe someone could have a look at it prior the proper 'template' is used to document the issue. Otherwise firestore will lose it's fans very soon.

Thank you
Michal

[MarkChrisLevy]

@goldensunliu @ihateids I reported similar issue #191 and firebase team just fixed it, maybe this fix will impact overall performance as well :-)

[mikelehen]

Thanks for the report. We are working on Firestore performance. Unfortunately I'm not sure if the fix for #191 will have an impact here. gets are implemented differently in the admin SDK and the JS SDK, so the performance difference isn't too surprising. We are aware that doing many gets concurrently in the JS SDK is slow and we're investigating some potential solutions that should help. I'm not sure when they'll be implemented, but be aware that it's on our radar.

[yankedev]

any news on this?
would it be better if instead of having multiple gets concurrently I have multiple listeners?

[mikelehen]

Internally, get() calls actually are the same as transient listeners (i.e. onSnapshot() calls) and so that won't make any difference.

There are two things in the works that could help here:

1. Some backend performance improvements that may help (though I'm not sure when they'll land or how big the impact will be).
2. We're looking into adding a getAll() method to the SDK that lets you pass multiple documents to read at once. This should be much more efficient than multiple get() calls. I am not certain when we'll get to this though.

[merlinnot]

Ad. 2, have you considered batching multiple get calls automatically/as an option when and if getAll lands? What I mean is to wait for some period of time after the first get call and batch all other calls of this method during this time. If someone sets this to a reasonable number (20, 50ms) this delay would possibly be compensated.

[mikelehen]

Yep! That is on our radar. It gets a little bit tricky because getAll() will fail all of the gets if security rules denies any one of them, and so if we start auto-batching when you do a get(), we are at risk of changing the behavior of get() [we'd likely need to fallback to unbatching them or something].

[merlinnot]

Unless getAll would just return an array of results/errors. You’re using this pattern in other services (sending notifications for example) so it would be more consistent across Firebase SDKs.

[mikelehen]

Unfortunately we can't do that easily. getAll() will essentially be a "batch get", similar to our "batch writes" and will be subject to the same all-or-nothing behavior. To change that would be a deep change on the backend which might defeat the performance gains we're attempting to achieve by introducing it in the first place.

[merlinnot]

I guess well designed app should make requests to data which it has no access to, so it shouldn’t be that much of an issue anyways :)

[yankedev]

I have been able to create an example to show my problem here:
https://stackblitz.com/edit/ionic-watdjp

using AngularFirestoreModule.enablePersistence() it decreases performances everytime I reload the page.

First time it takes less than 8seconds, the third time it takes more than 1 minute.
If I remove ".enablePersistence()" it is always fast as the first time.

Is there something I can look more in details?

[merlinnot]

@yankedev Hm, I can't reproduce the issue. As it seems to be a separate bug, could you please make a new issue and fill in the template (OS, browser, ...)?

Thank you for creating the minimal repro, it will make it so much easier for anyone to take a look 👍

[yankedev]

@merlinnot I changed my issue to a stackoverflow question https://stackoverflow.com/questions/48710244/how-can-i-speedup-firestore-or-angularfire-when-enablepersistence-is-active

it seems that with enablePersistence it is always slow instead of a performance degradation problem.
Thanks for your support

[merlinnot]

Ok, I assumed you've enabled persistence in your repro, my bad. Now I see the same results as you do. You should definitely open an issue for this.

[mikelehen]

Aha! Yes, after I changed your repro to enable persistence, I do see that on the second run (once there's data cached), there's a strange performance degradation (each successive query takes longer than the previous, even though they're all presumably against the same collection). Yes, please open a separate issue (and delete / resolve the SO post, since SO isn't a good place for reporting bugs).

[mikelehen]

There have been a number of performance improvements made since this bug was opened which may have closed the gap somewhat. Additionally, we have a separate feature request open to track adding getAll() to the Firestore SDK (#1176) which should close the gap the rest of the way.

[tmk1991]

@mikelehen - is there a limit to how many queries you can combineLatest/promise.all()/getAll() on?

If I loop through 2000 strings can I loop through and make 2000 queries where field = string?