BMA : améliorer le temps de réponse de `/tx/sources/:pubkey`

kimamila · 10 May 2023 13:33

Pour rendre Cesium a nouveau exploitable, en attendant la v2, je voudrais revoir optimiser les temps de réponse de /tx/sources/:pubkey

J’ai checkouté Duniter V1 (master) et identifié le code responsable :

   // TODO: very costly: needs a full scan, would be better to change this implementatio
    const entries = await this.findWhere((e) =>
      e.conditions.includes(`SIG(${pubkey})`)
    );
    const reduced = Indexer.DUP_HELPERS.reduceBy(entries, [
      "identifier",
      "pos",
    ]);
    return reduced.filter((r) => !r.consumed);

Sachant que findWhere() va ensuite appeler ce code :

this.db.createReadStream(options)
 (...)

=> A priori, il s’agit donc d’un “full scan” sur la table level_sindex

Pour éviter cela, je souhaite faire un index secondaire, puis le mettre à jour dans insertBatch et removeBlock()

@cgeek j’ai bon ? Sais tu s’il y a des cas de figure similaires, dont je pourrais m’inspirer. (je vais chercher)

Aussi, que me recommandes-tu pour les tests unitaires. Faut-il utiliser une BDD de test déjà prête, où bien m’en fabriquer une ?

cgeek · 10 May 2023 13:51

Oui je pense que c’est une bonne solution

Tu peux t’inspirer des sous-index comme indexForExpiresOn :

Pour les tests il y a déjà un fichier tout prêt, sources-dal.ts. Tu peux le copier pour créer ton propre test isolé, ou bien étendre ce fichier car c’est précisément le fichier qui teste les sources de monnaie.

kimamila · 10 May 2023 14:22

ok, vu.
D’ailleurs, il y a déjà des sous-index dans LevelDBSIndex.ts.

Tu n’utilises pas du tout subleveldown ? Ca vaut quoi a ton avis ?

import sub from "subleveldown";

init() {
  // ...
 this.pubkeyIndex = sub(db, "pubkey-index", { valueEncoding: "json" });

}

kimamila · 10 May 2023 14:31

Mais !?? je me trompe complètement où tout est déjà fait ?

il existe une fonction getAvailableForConditions() :

async getAvailableForConditions(
    conditionsStr: string
  ): Promise<SindexEntry[]> {
    const forConditions = await this.getForConditions(conditionsStr);
    const reduced = Indexer.DUP_HELPERS.reduceBy(forConditions, [
      "identifier",
      "pos",
    ]);
    return reduced.filter((r) => !r.consumed);
  }

Du coup il suffit e l’appeler pour bénéficier de l’index, non ?
La correction serait donc :

  async getAvailableForPubkey(
    pubkey: string
  ): Promise<
    {
      amount: number;
      base: number;
      conditions: string;
      identifier: string;
      pos: number;
    }[]
  > {
    const sources = await this.getAvailableForConditions(`SIG(${pubkey})`);
    return sources.filter((r) => !r.consumed);
  }

le test passe !

kimamila · 10 May 2023 14:35

EDIT: …oui, sauf que les conditions complexes ne seront pas récupérer, c’est ca ?

Dans le cas, je me dit que le plus simple est :

de détecter les conditions complexes, en les découpant dans un index par pubkey
puis de requeter :
- les conditions simple (cf ma solution plus haut)
- d’y ajouter les conditions complexes, depuis le nouvel index

Qu’en penses tu @cgeek ?

cgeek · 10 May 2023 14:46

Oui c’est exactement ça. Tu as tout compris.

A l’époque je n’ai simplement pas voulu faire cette différenciation vu la taille de la toile. Mais maintenant ça devient problématique

kimamila · 10 May 2023 15:58

ok du coup ca va le faire

Petite question sur le process de dev : faut-il toujours tout recompiler (avec cargo xtask build) à chaque modif de code, avant de relancer un test ?
(j’utilise WebStorm).
Tu n’aurais pas sous la main la configuration du “Run configuration” : par exemple un build incrémental en prérequis “Before Launch” du test ?

kimamila · 10 May 2023 16:42

EDIT : j’ai trouvé comment configurer “Before launch” => avec un simple tsc (ou npm run tsc).

Je vais donc pouvoir faire une MR. Depuis la branche dev du coup ?

kimamila · 10 May 2023 17:01

Voila ma MR : [fix] Optimize access to sources by pubkey, using specific index (!1420) · Merge requests · nodes / typescript / duniter · GitLab

@cgeek par contre je penses que la suppression est encore mal gérée… pourtant le TU passes.
C’est à cause du lien fait par const entries = await this.findByIdentifierAndPos(...) qui lui est bien vidé.

EDIT : c’est tout bon, pour la suppression.

kimamila · 11 May 2023 10:04

@Pini, si tu as moyen de déployer quelque part une version avec cette MR, histoire qu’on vérifie les temps de réponse ? Dans un premier temps sans faire de “reset data”. Cela va juste ignorer les TX avec condition complexe, mais ce n’est pas grave, car la plupart du temps les wallet clients ne les utilisent pas.
Par exemple :

/tx/sources/38MEAZN… sur g1.duniter.org : entre 22 et 25s
=> Nouveau temps ?

@cgeek, penses tu que je devrais faire une fonction de migration, qui remplirait le nouvel index à partir de celui sur les conditions, afin d’éviter des reset data ? As tu déjà fait cela quelque part ?

cgeek · 11 May 2023 18:02

Il y a bien une fonction de migration, mais je ne l’ai jamais utilisé pour les “tables” LevelDB.

C’est à toi de voir, comme tu le dis dans ton message précédent l’évolution que tu proposes est sans incidence sur le protocole pour les consommateurs classiques (paiements de compte à compte).

Pini · 11 May 2023 21:41

J’ai tenté de configurer mon noeud mirroir avec l’image générée à partir de cette MR, mais :

duniter_1  | Starting duniter with:
duniter_1  | /usr/bin/duniter direct_webstart
duniter_1  | internal/modules/cjs/loader.js:807
duniter_1  |   return process.dlopen(module, path.toNamespacedPath(filename));
duniter_1  |                  ^
duniter_1  | 
duniter_1  | Error: Error loading shared library ld-linux-x86-64.so.2: No such file or directory (needed by /duniter/neon/native/index.node)
duniter_1  |     at Object.Module._extensions..node (internal/modules/cjs/loader.js:807:18)
duniter_1  |     at Module.load (internal/modules/cjs/loader.js:653:32)
duniter_1  |     at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
duniter_1  |     at Function.Module._load (internal/modules/cjs/loader.js:585:3)
duniter_1  |     at Module.require (internal/modules/cjs/loader.js:692:17)
duniter_1  |     at require (internal/modules/cjs/helpers.js:25:18)
duniter_1  |     at Object.<anonymous> (/duniter/app/lib/logger.js:16:18)
duniter_1  |     at Module._compile (internal/modules/cjs/loader.js:778:30)
duniter_1  |     at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
duniter_1  |     at Module.load (internal/modules/cjs/loader.js:653:32)

EDIT : l’exécutable incriminé /duniter/neon/native/index.node n’est pas issu de la compilation. Il a été intégré au dépôt git sous forme binaire déjà compilé par Éloïs. Et il a été manifestement compilté avec la glibc, non disponible sur Alpine Linux qui utilise musl. Ce dernier point explique l’erreur.

Ce qui reste à comprendre c’est :

À quoi sert cet exécutable ?
Comment peut-on le recompiler ?
Pourquoi il ne semble nécessaire au fonctionnement de Duniter que suite à la MR de @kimamila.

Que de questions sans réponses…

En mode pragmatique, et dans l’hypothèse où cet exécutable est incontournable, on peut basculer l’image Docker sur une base Debian qui aura plus de chance d’être compatible avec lui.

EDIT 2: ma proposition “pragmatique” ci-dessus est débile. Ça ne résout le problème que sur architecture x86_64. On n’a pas de build arm64 de cet exécutable.

EDIT 3 : L’heure est grave. J’ai tenté un git bisect pour trouver le commit coupable, mais tout échoue. J’ai l’impression que c’est juste l’environnement de build qui a changé (version de rustup ou d’une dépendance ?). Car j’ai beau remonter aussi loin que je veux dans le temps, toutes les anciennes versions fraichement rebuildées plantent. À ce stade un oeil de développeur serait bienvenu (@cgeek @tuxmain ?).

EDIT 4 : Ouf ! Tout va bien. Manifestement j’ai mis le bazar dans mon répertoire de travail avec de précédentes tentatives de build in place. De ce fait il subsistait des artifacts de build dans l’arborescence des sources qui n’étaient pas repérés par git (car dans .gitignore) mais qui se retrouvaient dans la copie de l’arborescence au moment du build docker. La suppression de tous les fichiers générés a permis de corriger le souci.

Pour éviter ces déboires à l’avenir je vais proposer une MR avec inclusion du contenu de .gitignore dans .dockerignore.

@kimamila : je viens de mettre à jour mon noeud mirroir duniter.pini.fr avec ta MR.

kimamila · 15 May 2023 09:41

Wahoo, cool !

Alors voila le résultat : https://duniter.pini.fr/tx/sources/38MEAZN68Pz1DTvT3tqgxx4yQP6snJCQhPqEFxbDk4aE réponse 296ms ! Soit un facteur d’optimisation x100 !
Ca roule donc comme prévu. Reste à attendre le merge de @cgeek

La suite ?

Maintenant, pour optimiser pleinement l’affichage de “Mon compte” dans Cesium, il faut optimiser /wot/requirements/:search qui réponds en 30s.

J’ouvre un nouveau post pour en parler

cgeek · 15 May 2023 19:19

MR approuvée.

Pini · 16 May 2023 10:57

@cgeek on peut merger du coup ? J’ai toujours un doute quand l’approbateur ne déclenche pas lui-même le merge

cgeek · 16 May 2023 11:22

Oui vous pouvez

kimamila · 31 May 2023 12:54

J’ai reporté mes modifs dans la MR !1428 pour Duniter 1.8. On devrait avoir exactement les même temps de réponse.

Veux tu me relire @cgeek ? A priori rien ne change vis à vis de l’autre MR sur la branche dev

cgeek · 2 June 2023 19:33

Je ne comprends, on dirait la MR [fix] Optimize access to sources by pubkey, using specific index (!1420) · Merge requests · nodes / typescript / duniter · GitLab que j’ai déjà relue et qui a été mergée.

En tout cas j’ai du mal à suivre il y a trop de code d’un coup.

Si je ne peux pas suivre il vaut mieux faire sans moi, tant pis je ne relirai le code qu’à posteriori.

kimamila · 2 June 2023 19:59

Oui, c’est un report mais sur la branche 1.8. je te l’ai indiqué dans les commentaires de la MR.