[DAT-74] feat: Do not rebuild search index from scratch #2729
Conversation
paultranvan
requested review from
JF-Cozy,
Merkur39,
Ldoppea,
zatteo and
doubleface
as code owners
paultranvan
force-pushed
the
import-export
branch
2 times, most recently
from
96fa493 to
a445274
Compare
| for (let attempt = 0; attempt < maxRetries; attempt++) { | ||
| const searchResults = await dataProxy.search(searchValue) | ||
|
|
||
| if (searchResults && Array.isArray(searchResults)) { |
There was a problem hiding this comment.
If the search is unsuccesful, what can it return ? null ? Or something else ?
I find it strange to need to check if is an array.
There was a problem hiding this comment.
If the index are not initialized, null will be returned
If the index are initialized, and the search is working it will be an array
There was a problem hiding this comment.
So theoretically no need to check Array.isArray(searchResults) ? But it's a nit, no problem.
There was a problem hiding this comment.
Yes, I guess we could just do if (searchResults) and avoid an additional check 👍
There was a problem hiding this comment.
Fixed within 344c30c
| const searchWithRetry = async ( | ||
| dataProxy, | ||
| searchValue, | ||
| { maxRetries = 5, delay = 500 } = {} |
There was a problem hiding this comment.
500 is maybe an high delay for the first attempt ? But I don't know what time can this kind of thing take.
There was a problem hiding this comment.
The very first attempt will be right away. It is actually the second attempt that will make 500ms. The performances measures in the PR description give some insights of the time to have initialized index
There was a problem hiding this comment.
Let's try with this and we'll see.
|
Nice! I like the backoff. |
| if (searchIndex) { | ||
| this.searchIndexes[doctype] = searchIndex | ||
|
|
||
| if (this.isLocalSearch) { |
There was a problem hiding this comment.
there seems to be no code branch for the remote search anymore, is that expected?
Also those are 2 consecutive if(this.isLocalSearch), should we merge theme?
There was a problem hiding this comment.
This is definitely not expected, good catch!
Fixed within 0443ed6
Each time a searchEngine is instanciated, we used to rebuild the search indexes from scratch, i.e. query the local database and compute the indexes. This can be quite time consuming, especially in slow mobile devices. Therefore, we now export the search indexes on disk, and import them at init time. We save the lastSeq for each index, to be the starting point of the next changes batch updates, fetched from the local database after a remote replication. Some measures, performed on a 8K files database: **Redmi Note 10** - Create index: without peristance - old way - Query all io.cozy.files : 3700ms - Build files index : 6500ms - **Total: 10200 ms** - Create index: with perstance - new way - **Import persisted index: 800 ms** - Export index: 1057ms **OnePlus Nord 4** - Create index: without peristance - old way - Query all io.cozy.files : 2000ms - Build files index : 2900ms - **Total: 4900 ms** - Create index: with perstance - new way - **Import persisted index: 332 ms** - Export index: 363ms If we compare the query+build index time w.r.t the import time, we have a ~13x time improvement before the index is ready to use.
paultranvan
force-pushed
the
import-export
branch
2 times, most recently
from
fc59d65 to
51abf16
Compare
Sometimes the search might return nothing because the indexes are not ready yet. In such case, the user will get no search result and will have to retry. To avoid this, we now wait for the index to be ready by making several retries, with exponential backoff
paultranvan
force-pushed
the
import-export
branch
from
51abf16 to
344c30c
Compare
paultranvan
changed the title
Since cozy/cozy-libs/pull/2729 the SearchEngine requires the consuming app to provide a storage provider in order to persist its index Related PR: cozy/cozy-libs#2729
Since cozy/cozy-libs/pull/2729 the SearchEngine requires the consuming app to provide a storage provider in order to persist its index Related PR: cozy/cozy-libs#2729
Since cozy/cozy-libs/pull/2729 the SearchEngine requires the consuming app to provide a storage provider in order to persist its index Related PR: cozy/cozy-libs#2729
Since cozy/cozy-libs/pull/2729 the SearchEngine requires the consuming app to provide a storage provider in order to persist its index Related PR: cozy/cozy-libs#2729
Each time a searchEngine is instanciated, we used to rebuild the search
indexes from scratch, i.e. query the local database and compute the
indexes. This can be quite time consuming, especially in slow mobile
devices. Therefore, we now export the search indexes on disk, and import
them at init time. We save the lastSeq for each index, to be the
starting point of the next changes batch updates, fetched from the local
database after a remote replication.
Some measures, performed on a 8K files database:
Redmi Note 10
Query all io.cozy.files : 3700ms
Build files index : 6500ms
Total: 10200 ms
Import persisted index: 800 ms
Export index: 1057ms
OnePlus Nord 4
Query all io.cozy.files : 2000ms
Build files index : 2900ms
Total: 4900 ms
Import persisted index: 332 ms
Export index: 363ms
If we compare the query+build index time w.r.t the import time, we have
a ~13x time improvement before the index is ready to use.