perf: improve perf inside search #2663

trollepierre · 2022-08-05T09:52:38Z

Needs:
feat(stack-client): Add findAll method : cozy/cozy-client#1219
feat(SearchBar): call onSelect when it's provided : cozy/cozy-bar#770
feat(SearchBar): call onSelect when it's provided : cozy/cozy-bar#771

Changelog updated if needed
Benchmark to do

src/drive/web/modules/services/components/SuggestionProvider.jsx

src/drive/web/modules/queries.js

src/drive/web/modules/services/components/SuggestionProvider.jsx

bundlemon · 2022-08-08T13:25:43Z

BundleMon

Files updated (5)

Status	Path	Size	Limits
✅	public/drive.(hash).js	1.45MB (+2.3KB +0.16%)	1.6MB
✅	vendors/drive.(hash).js	1.39MB (+2.01KB +0.14%)	1.54MB
✅	services/dacc/drive.js	248.36KB (+1.49KB +0.6%)	249KB
✅	services/qualificationMigration/drive.js	243.25KB (+1.48KB +0.61%)	244KB
✅	app/drive.(hash).js	233.36KB (+95B +0.04%)	234KB

Unchanged files (5)

Status	Path	Size	Limits
✅	public/pdf.worker.entry.(hash).worker.js	220.57KB	221KB
✅	public/drive.(hash).min.css	85.44KB	86KB
✅	app/drive.(hash).min.css	54.51KB	55KB
✅	vendors/drive.(hash).(hash).min.css	34.34KB	35KB
✅	public/cozy-bar.css	18.42KB	24KB

Total files change +7.39KB +0.18%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

paultranvan · 2022-08-09T14:14:02Z

src/drive/web/modules/queries.js

+        $ne: TRASH_DIR_ID
+      },
+      path: {
+        $or: [{ $exists: false }, { $regex: '^(?!/.cozy_trash)' }]


nit: as this predicate on the path shouldn't be necessary if the trash was more reliable, it would be worth addding a comment here to mention it

src/drive/web/modules/queries.js

src/drive/web/modules/services/components/SuggestionProvider.jsx

trollepierre · 2022-08-10T15:17:23Z

src/drive/web/modules/services/components/helpers.js

  if (isDir) {
    path = file.path
    url = urlToFolder
  } else {
    const parentDir = folders.find(folder => folder._id === file.dir_id)
    path = parentDir && parentDir.path ? parentDir.path : ''
    if (models.file.isNote(file)) {
-      url = await models.note.fetchURL(client, file)
+      onSelect = () => models.note.fetchURL(client, file)


@acezard : here is the call of the function (unique, in cozy)

What do we do with the return of this fetchURL() method?

Because if we click on the suggestion, it'll call onSelect: cozy/cozy-bar@7b7491d#diff-bd7d5c5cdb7c2b6d78ffe2d7d7045e90beba76f929eff8695969c06a4d1accc7R237

Then onSelect make the call to fetchURL() but we do we do with the result? There is no window.location or else, how does it works to open the note?

la définition de isNote me dérange un peu :

export const isNote = file => { if ( file && file.name && file.name.endsWith('.cozy-note') && file.type === FILE_TYPE && file.metadata && file.metadata.content !== undefined && file.metadata.schema !== undefined && file.metadata.title !== undefined && file.metadata.version !== undefined ) return true return false }

mais comme je récupère moins de données, les notes ne sont pas considérées comme telles (par cette ligne du helper et aussi le getIconUrl plus bas).

Il faut choisir :

ajouter les metadata

changer de condition isNote

Je vais partir sur l'ajout des metadata, en espérant qu'elles ne soient pas trop lourdes

ps:
PRs de fix de cozy-bar:

feat(SearchBar): fetch and open note once clicked on a suggestion cozy-bar#773

feat(SearchBar): fetch and open note once clicked on a suggestion cozy-bar#774

j'ai ajouté un nouveau commit pour fixer ceci.

metadata peut être assez volumineux, ça contient les qualifications, les infos EXIF, etc : https://github.com/cozy/cozy-doctypes/blob/master/docs/io.cozy.files_metadata.md
C'est un peu dommage de devoir rapatrier tout le bloc juste pour savoir si c'est une note. Un file.mime === 'text/vnd.cozy.note+markdown' n'est pas suffisant @Crash-- ?

Et sur un autre sujet, ça m'embête de voir que :

les metadata des notes (content, schema, title, version) sont à la racine de metadata. J'aurais trouvé mieux de grouper tout ça dans un objet metadata.notes, ça devient un sacré bazard les metadata.

Je ne vois nul part documenté ces attributs : https://github.com/cozy/cozy-doctypes/blob/master/docs/io.cozy.files_metadata.md

et un deuxième, car le postMessage serialize la fonction onSelect, je suis donc obligé de réaliser le fetchUrl directement depuis la cozy bar

les metadata des notes (content, schema, title, version) sont à la racine de metadata. J'aurais trouvé mieux de grouper tout ça dans un objet metadata.notes, ça devient un sacré bazard les metadata.

Je ne sais même pas si metatada se révèle être le bon endroit pour mettre le contenu de la note au final. Il me semble que ce n'est pas la première fois qu'on nous remonte une incompréhension / souci avec. Mais il est vrai que d'avoir tout ce qui est propre à la note scopé quelque part serait plus simple d'utilisation...

Je ne vois nul part documenté ces attributs : https://github.com/cozy/cozy-doctypes/blob/master/docs/io.cozy.files_metadata.md
cozy/cozy-doctypes#208

C'est un peu dommage de devoir rapatrier tout le bloc juste pour savoir si c'est une note. Un file.mime === 'text/vnd.cozy.note+markdown' n'est pas suffisant @Crash-- ?

L'idée était d'être sur qu'on n'essaye pas d'ouvrir l'éditeur avec du contenu qu'il ne sait pas gérer. On peut pas juste sélectionner [metadata.content, metadata.schema...]?

Selector is mandatory when using .select or .partialIndex cozy/cozy-client#1216

Crash-- · 2022-08-16T09:13:44Z

src/drive/web/modules/services/components/SuggestionProvider.jsx

-        !file.trashed && !/^\/\.cozy_trash/.test(file.path)
-      const notOrphans = file =>
-        folders.find(folder => folder._id === file.dir_id) !== undefined
+    const folders = files.filter(file => file.type === TYPE_DIRECTORY)


Since we can benefits from http2 multiplexing, I'm wondering if we should not make two requests:

one for the files

one for the folders

We'll have the almost the same number of round trips (1 or 2) but we'll have a few of them parallelized. @paultranvan what do you think?

I assume that, most of the time, the number of files will be order of magnitudes higher than the number of folders. So, the folder query might end up sooner than the files one, which is good, as we want to display the folders first. The downside is that we have now 2 indexes to compute instead of one, which might slow down the search when we make a query with non-indexed docs.
I think what you propose is still relevant either way, as it should allow to get partial (i.e. folders) results faster and I think faster user response is more important than the overall performance.
That being said, I also think paginated mango queries is sub-optimal for this use-case :/ I look forward for view-base approach and/or pouchdb exploration

Crash-- · 2022-08-16T09:17:31Z

test/dummies/dummyFile.js

+ * Create a dummy file, with overridden value of given param
+ *
+ * @param file
+ * @returns {*&{path: string, name: string, icon: string, id: string, _id: string, dir_id: string, type: string}}  a dummy file


What kind of jsdoc this is?

this is generated by my IDE.

* means any type. (because i didn't set the type of the file)

& means intersection

I will reformulate.

I don't see anything related to this syntax on https://jsdoc.app/ . Can you point me the doc please?

I don't know exactly this syntax. My IDE generates it.

About: Allows any type

/** * @param {*} somebody - Whatever you want. */

I guess about intersection:
https://www.typescriptlang.org/docs/handbook/2/objects.html#intersection-types

Crash-- · 2022-08-17T13:14:06Z

src/drive/web/modules/queries.js

+    _id: {
+      $gt: null
+    }
+  }


@trollepierre after the merge of cozy/cozy-client#1225 you should be able to remove this selector

Crash-- · 2022-08-22T07:24:31Z

src/drive/web/modules/queries.js

+    'path',
+    'type',
+    'mime',
+    'metadata'


Une chose est sûre, c'est qu'on peut pas garder metadata au complet. Soit on arrive à aller lire quelques champs particuliers, soit on change la façon de checker si c'est une note. Car metadata sur les notes, ça peut vite peser des centaines de ko par note.

j'ai recréé un helper isNote, car les metadata peuvent être volumineuses.

I don't know if I like this idea.

You can add to the select(['metadata.title', 'metadata.version']) . title + version are not so big. So the helper can be if(metata.title && metadata.version && file.endsWith... && ...) .

See a test query with metadata.datetime to see the result.

What do you think?

I have only one note in my local Drive.
When adding:

'metadata.content', 'metadata.schema', 'metadata.title', 'metadata.version'

as fields, the size of request increase
from 650ko / 39,87 ko transferred in 538ms
to 657,97 ko / 41,61ko transferred in 578ms.

That is quite huge, I think.

This is not what I suggested. I suggested to only add version + title. Not schema + content.

perfect

PR updated

…hing we don't fetch all metadata param of file, because this field may make requests much bigger that's why we need to create a specific method to fix this problem

Crash-- · 2022-08-22T13:31:08Z

src/drive/web/modules/services/components/helpers.js

+    file &&
+    file.name &&
+    file.name.endsWith('.cozy-note') &&
+    file.type === FILE_TYPE


you can edit this helper since you have more informations now (metadata.title & metadata.version)

I'm wondering if we should not change the helper from cozy-client directly instead of having this one in Drive.

6 usages in Cozy:

=> 5 in Drive, 1 in Cozy UI :

https://cs.github.com/?scope=org%3Acozy&scopeName=cozy&q=models.file.isNote%28

I know and this is exactly my point... We learnt that requesting metadata.content it's big. So we should not request it anymore. And we should not relying on it to know if the files is a note or not a note. No?

Crash-- · 2022-08-23T13:39:28Z

@trollepierre Can you split this PR in two PR? One that remove the request for the note, and the other that change the all_docs to /data/io.cozy.files/_find ?

I think that the first one can be merged without the second one.

trollepierre requested review from Crash--, paultranvan and JF-Cozy August 5, 2022 09:52