Project

General

Profile

Enhancement #4214

Optimering af søgehastighed ved gennem afkobling af entity model

Added by Kasper Garnæs 8 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Urgent
Assignee:
Estimated time:
URL med eksempel:
Kategorier:
Driftsvedligehold - Refaktorering (Opdatering af kodebasen), Søgning - Søgeresultat efter søg - Brønd

Description

Når materialer vises i DDB CMS benyttes Drupals entity model som ramme for at styre:

  1. Hvilke data (felter) der findes for hvert materiale
  2. Hvordan de skal vises (udvælgelse og rækkefølge)

Denne model er kompleks at beregne og kræver potentielt set yderligere kald til OpenSearch, som pt. forsøges omgået igennem caching. Alt i alt medfører det længere svartider.

Historisk har det vist sig begrænset hvor meget disse features er blevet brugt i forbindelse med visning af søgeresultater.

På den baggrund har Árni Loftsson fra Randers Bibliotekerne foreslået at reducere svartiden ved søgeresultater ved at afkoble visningen af disse fra entity modellen.

Afkoblingen skal tage hensyn til at systemet ud over DDB CMS også bliver brugt i andre sammenhænge fx. eReolen.dk, og at disse sites muligvis bruger entity modellen til tilpasning af deres resultater.


Related issues

Related to DDB CMS - Bug #4295: Deaktivér ratings på karuseller og søgeresultatsidenReviewed
Related to DDB CMS - Bug #4296: Lazy load (AJAX) ratings for forbedret performanceResolved (tag version)

History

#1 Updated by Kasper Garnæs 8 months ago

  • Kategorier Søgning - Søgeresultat efter søg - Brønd added


Jeg har arbejdet videre på forslaget her for at vise hvordan det kunne fungere i praksis: https://github.com/ding2/ding2/pull/1386.

Der er som jeg ser det to centrale ændringer, som jeg gerne vil have feedback på:

1. Introduktionen af en theme hook til rendering af søgeresultater: https://github.com/ding2/ding2/pull/1386/commits/00949a484d2c97c2f09b8ee304fa7dd038c62017

2. Tilgangen hvor vi abstraherer dataudtræk fra materialer væk fra feltvisning. Eksempel med serieinformation: https://github.com/ding2/ding2/pull/1386/commits/54d8219c3be88114f8c2c7064735cc041023bdb5

 

Ovenstående er kun ment som en start. I forhold til det implementerede manger der stadig afdækning af en række af de mere komplekse felter bl.a.: tilgængelighed, forsider og ratings. Jeg forventer at de vil følge samme struktur som serierne.

#2 Updated by Árni Loftsson 8 months ago

Jeg synes det ser rigtig godt ud og en god måde at bypasse entitetsmodellen på. Jeg synes brugen af theme hook er god løsning som skaber fleksibilitet. Jeg kan også rigtig godt lide at vi abstraherer dataudtræk fra materialer væk fra feltvisning. Det er et rigtigt godt skridt i retningen af skille forretningslogiken fra visningslogikken. Det gør hver form for migration til ny Drupal eller andet system nemmere. Og som en som har brugt rigtig meget tid på lede efter funktionalitet i DDBCMS bare det at metoden nu er navngivet i relation til det den gør er en kæmpe hjælp.

#3 Updated by Rolf Madsen 8 months ago

  • Status changed from New to Development
  • Priority changed from Normal to Urgent
  • Target version set to Release 31 - Adgangsplatform og LUG

Det prioriteres at gå videre med udviklingsopgaven da vi ser et stort potentiale i den løsning der er skitseret.

#4 Updated by Gitte Barlach 7 months ago

Jeg håber at bypass af entity modellen vil løse de svartidsproblemer Daniel omtaler i #4213:

"Det tager lang tid for siden et blive indlæst men det og jo også en del af problemet. Dog får man nu vist alle wimpy kid efter ca 30 sekunders ventetid ;-)"

#5 Updated by Kasper Garnæs 7 months ago

  • Status changed from Development to Needs design decision
  • Assignee changed from Kasper Garnæs to Gitte Barlach

Jeg er nu færdig med et PR med en løsning på dette: https://github.com/ding2/ding2/pull/1386

Min implementation af bypass af entitymodellen ved visning af søgeresultater giver en ~7% forbedring af svartider ved ucachede søgeresultater (5.33s vs. 5.69s).

Gevinsten er lavere end jeg forventede, inden jeg gik i gang. Jeg vil derfor gerne bede en eller flere andre udviklere om at prøve at gennemføre testen i deres egne miljøer samt udfordre testmetoden såvel som implementationen. Hvis genvisten ikke er større er spørgsmålet for mig overordnet set om ændringen er den øgede kompleksitet værd? Det betyder i praksis at vi skal holde to forskellige søgeresultatsvisninger synkroniseret.

Opgaven er oprindeligt baseret på en hypotese om at entitymodellen medførte et unødvendigt performanceoverhead. Resultatet fik mig til at overveje hvorfor det kunne være. Én forskel er at den oprindelige implementation endnu ikke dækkede visning af ratings fra OpenList. Data fra andre eksterne systemer der er i spil i forbindelse med søgeresultatvisningen (Covers og Availability) bliver hentet asynkront men så bliver data for ratings hentet synkront. Jeg har derfor også foretaget en måling hvor jeg fjerner ratingsvisningen fra det opdaterede resultat.

Fjernelse af ratings giver en forbedring af svartider på >40% (3.11s vs. 5.69s).

På den baggrund foreslår jeg at vi implementerer asynkron hentning af ratings fra OpenList. Dette kan foregå uafhængigt af en beslutning af om vi vil fortsætte med dette arbejde eller ej.

Konkrete svartider og metode kan ses i PR'et.

#6 Updated by Gitte Barlach 7 months ago

Super godt arbejde, Kasper. Vigtige konklusioner ift. ratings. Ifm med #4175 er det også den konklusion Fini kommer frem til:

"Det smarteste ville være hvis rating widget'en fungerede som covers og satte en placeholder ind hvis den ikke fandt noget i cachen, og så loadede ratings over AJAX (og så ville bulk forespørgelser være smart, men det er en anden snak). Men det er ikke bare lige et fem minutters fix."

Og derfor har jeg opdateret den sag med dine konklusioner her. 

ift. test, testmetode samt implementation har jeg bedt Jesper, Jørgen og Simon om at se på dette. Andre udviklere er også meget velkomne til at gå det efter i sømmene!

#7 Updated by Rolf Madsen 7 months ago

  • Related to Bug #4295: Deaktivér ratings på karuseller og søgeresultatsiden added

#8 Updated by Rolf Madsen 7 months ago

  • Related to Bug #4296: Lazy load (AJAX) ratings for forbedret performance added

#9 Updated by Thomas Hansen 7 months ago

Den store ide i at bruge entities og fields til ting objekter var at det gav en flexibilitet i forhold til hvad og hvordan man presenterede materialer.Ved at det hele var fields så kunne det være muligt for site administratoren at vælge hvad der blev vist i Drupals field UI, og til en hvis grad også hvordan (med field formatters).

Men jeg har gentagende gange nævnt at det ikke giver så meget mening med DDB CMSs one-size-fits-all mentalitet, hvor det ville give mere mening bare at lange et simpelt objekt (måske TingObject) over til en theming funktion (som Ereolen så kunne customize en smule).

Tilsyneladende er det ikke CPU performance der er det store problem, men abstraheringen ud i felter giver en hel del udvikler overhead, for det er ikke "bare lige" at tilføje et nyt stykke information på objektet (og der er rigelige muligheder for at skyde sig selv i foden, som rating demonstrerer).

Så istedet for at tilføje endnu et kompliceringslag, så vil jeg anbefale at takle problemet ordenligt og beslutte sig for om fields skal rives ud og det skal re-arkitekteres til noget simplere materiale håndtering.

#10 Updated by Árni Loftsson 7 months ago

Godt arbejde Kasper. Vi fjernede i dag ratings fra vores søgeresultater i dag. Før vi fjernede dem tog det typisk i mellem 3 - 3.5 sekunder at hente søgeresultater. Efter at vi fjernede dem er vi nede på 1,8 - 2,5 sekunder. Dvs. en markant forbedring som svarer meget godt til det 40% tal som Kasper kom frem til og ca. til den gevinst jeg målte når jeg bypassede entitetsmodellen. Hvis i vil teste det i praksis gå til randersbib.dk .

Samtidigt viser hele forløbet det underliggende problem med entitietsmodellen og den kompleksitet den skaber. Det kræver at en ekspert som Kasper dykker dybt ned i koden for at finde en fejl som den her. Så det er klart at entitetsmodellen skal væk på længere sigt. Jeg kan godt leve med at det først sker når vi skifter over til Drupal 9 når vi nu har løst de største performance problemer.

Jeg vil anbefale at de mekanismer til at bypasse entitetsmodellen som Kasper har lavet bliver bibeholdt. Der kan være andre usecases hvor det giver mening at bruge dem.

#11 Updated by Simon Holt 7 months ago

Ja, super arbejde og virkelig spændende finding med ratings. Måske vil asynkrone ratings også forbedre performance på statuslisterne, da ratings også vises der.

Jeg har prøvet at installere lokalt og teste (samme test som Kasper nævnt i PR) og kommer stort set frem til det samme som Kasper. Dog med lidt lavere performance-forberinger uden ratings, men stadig meget godt:

  • Ting search fast disabled: 4%
  • Ting search fast: 10%
  • Ting search fast no ratings: 30%
  • Master no ratings: 28% 

Er enig med Thomas og Arni i at entitetsmodellen skal helt væk på længere sigt, men tænker det måske ikke er besværet værd på nuværende tidspunkt og at det vil give mening at ifb med overgang til Drupal 9 (eller hvad der nu kommer til at ske).

#12 Updated by Simon Holt 7 months ago

Besluttede også at deaktivere ratings på søgeresultatssiden på vejlebib.dk. Det kan gøres meget nemt under admin/structure/ting_object/display/search_result ved at sætte "Rating"-gruppen til skjult.

Her er nogle tal fra vores produktions server med/uden ratings:

Med ratings:
3,334 sekunder
3,154 sekunder
3,409 sekunder
3,235 sekunder
3,677 sekunder
3,859 sekunder
3,241 sekunder
2,917 sekunder
3,252 sekunder
3,423 sekunder
Gennemsnit: 3,3501 sekunder

Uden ratings
2,482 sekunder
3,156 sekunder
2,834 sekunder
1,803 sekunder
2,156 sekunder
2,182 sekunder
2,337 sekunder
2,777 sekunder
2,924 sekunder
2,123 sekunder
Gennemsnit: 2,4774 sekunder

Performance forbedring ((3,3501 - 2,4774) / 3,3501) x 100 = 26,05%

Testet med:

for i in {1..10}; do drush cc all > /dev/null && drush -l stg.vejlebib.dk vpa > /dev/null && curl https://stg.vejlebib.dk/search/ting/tintin -s -o /dev/null && curl https://stg.vejlebib.dk/search/ting/danmark -s -o /dev/null -w  "%{time_starttransfer}\n"; done
 

#13 Updated by Jesper Kristensen 7 months ago

Kørt i docker setup med aakb cms (hvilket er DDB core + nogle få moduler og patches).

Ud fra dette giver fast search ikke meget til slut resulatatet, men der imod giver disabled af ratings en hel del.

#14 Updated by Simon Holt 7 months ago

Bemærkelsesværdigt at du kun får under 1% med fast search. Jeg fik alligevel omkring en 10% forbedring.

 

#15 Updated by Kasper Garnæs 7 months ago

PR på disabling af search results: https://github.com/ding2/ding2/pull/1418 (selv om jeg ikke ved om det hører specifikt til i dette issue).

#16 Updated by Christel Krabbenhøft 7 months ago

  • Status changed from Needs design decision to Needs code review

#17 Updated by Gitte Barlach 7 months ago

Dejligt, mange tak!
Orker du at flytte dit PR hen på https://platform.dandigbib.org/issues/4295 ?

#18 Updated by Gitte Barlach 7 months ago

  • Assignee changed from Gitte Barlach to Jørgen Nielsen

#19 Updated by Jesper Kristensen 7 months ago

FYI: så kigger jeg på at ajax load rateings i https://platform.dandigbib.org/issues/4296

#20 Updated by Jørgen Nielsen 6 months ago

  • Status changed from Needs code review to Reviewed - Needs info/rework
  • Assignee changed from Jørgen Nielsen to Kasper Garnæs

Lidt nitpicking, ellers fint. Men der er også opstået en merge konflikt.

#21 Updated by Kasper Garnæs 6 months ago

  • Status changed from Reviewed - Needs info/rework to Development
  • Assignee changed from Kasper Garnæs to Jesper Kristensen

Efter at rating ny hentes asynkront jf. #4296 handler den videre diskussion nu kun om afkobling af entity modellen.

Jeg har rebaset PR'et men ikke adresseret yderligere kommentarer fra reviewet eftersom det afhænger af hvorvidt vi ønsker at gå videre med løsningen.

 

Vores performacetests af direkte afhængige af OpenSearch performance (inkl. variationer af denne). For at kunne udelukke dette har vi snakket om at mocke kald til OpenSearch ud således at vi kan få nogle "rå" tal ift. hvilken effekt afkoblingen har.

#22 Updated by Kasper Garnæs 5 months ago

  • Status changed from Development to Needs design decision
  • Assignee changed from Jesper Kristensen to Rolf Madsen

Jeg har testet løsningen ved at afkoble afhængigheden til OpenSearch vha. WireMock og afkoble netværksadgang. Her er jeg kommet frem til følgende resultater baseret på 50 testkørsler:

  1. Master: 1,48s
  2. Ting search fast branchen (Afkoblingen slået fra): 1,40s
  3. Ting search fast branchen (Afkoblingen slået til): 1,26s

Der bør ikke være væsentlig forskel på master og branchen med ting search fast. Det kunne tyde på at der stadig er en smule usikkerhed i praksis.

Med det i peger mine resultater på at afkoblingen af entitymodellen sparer 0,06s til 0,14s.

#23 Updated by Rolf Madsen 3 months ago

  • Status changed from Needs design decision to Closed
  • Target version changed from Release 31 - Adgangsplatform og LUG to Release 30-1 - Place2Book (7.x-4.7.0)

Formålet med dette issue, nemlig at opnå forbedringer på svartiden i søgeresultatet, blev løst via https://platform.dandigbib.org/issues/4295 og https://platform.dandigbib.org/issues/4296.

Derfor lukkes dette issue.

Tak for indsatsen til alle der har bidraget med at sætte fokus på problemstillingen samt analyse og udvikling af samme!

Also available in: Atom PDF