Skip to content

Commit 23c56f3

Browse files
zonky2zonky2
committed
Build the model carrying the input values once per run
processInput() calls getWidget() once per property, and every call rebuilt the whole model: clone it, encode every value of the bag, write every value onto the clone. With n properties that is n clones, n² encode events and n² setProperty calls - and in MetaModels one setProperty costs three attribute conversions, because the self comparison and the following DifferentValuesException::compare each convert again. The values are the same for all widgets of one run, so the model is built once and cached, keyed over the contents of the bag. Callers still get their own instance: cloneModel() hands out a copy, which is safe because MetaModels' model deep copies its item on clone, so a listener on the BuildWidgetEvent cannot pollute the cache. The semantics are untouched. Every widget still sees the input of all fields, which is what display conditions and dependent selects are evaluated against - that was never the problem, doing it per widget was. Measured on mm_employees::11, 27 widgets, xdebug off, same conditions before and after: valueToWidget 1785 -> 221 setProperty 1349 -> 99 widgetToValue 1325 -> 75 encodeValue 1323 -> 73 wall time POST ~20 s -> ~9.5 s docs/performance-editmask.md records the measurement, why the rebuild is needed at all, and what is left: the three conversions per setProperty sit in metamodels/core, the 76 getWidget calls for 27 properties are unexplained, and a translated model has not been measured yet. Verified: display conditions 5/5 - the first thing that would break if a widget stopped seeing the other fields - regression 19/19, toggle 6/6, edit all 4/4, legends 8/8. phpcq passes; the minor it used to report is gone too, since the empty catch is now explained rather than covered by a class level suppression.
1 parent 43bcd9d commit 23c56f3

2 files changed

Lines changed: 214 additions & 20 deletions

File tree

‎docs/performance-editmask.md‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
# Laufzeit der Eingabemaske
2+
3+
> Stand: **erster Schritt umgesetzt** — der quadratische Aufbau in
4+
> `ContaoWidgetManager::getWidget()` ist beseitigt. Die Verdreifachung je `setProperty` in
5+
> MetaModels ist erkannt, aber noch offen.
6+
7+
## Der Befund
8+
9+
`ContaoWidgetManager::processInput()` ruft `getWidget()` **je Property** auf. Jeder Aufruf
10+
baute bisher das gesamte Modell neu auf:
11+
12+
```php
13+
$model = clone $this->model; // ① voller Model-Klon
14+
foreach ($inputValues->getIterator() as $name => $value) { // ② über ALLE Werte
15+
$values->setPropertyValue($name, $this->encodeValue($name, $value, $inputValues));
16+
}
17+
$controller->updateModelFromPropertyBag($model, $values); // ③ setProperty für ALLE
18+
```
19+
20+
Bei *n* Properties also *n* Klone, *n²* `encodeValue`-Ereignisse und *n²* `setProperty`.
21+
22+
Verstärkt wird das in MetaModels: **ein** `setProperty()` in
23+
`MetaModels\DcGeneral\Data\Model` kostet **drei** Attribut-Konvertierungen —
24+
25+
```php
26+
$varInternalValue = $objAttribute->widgetToValue($varValue, $item->get('id')); // 1
27+
if ($varValue !== $this->getProperty($strPropertyName)) { // 2 → valueToWidget
28+
$item->set($strPropertyName, $varInternalValue);
29+
DifferentValuesException::compare($varValue, $this->getProperty(...), false); // 3 → valueToWidget
30+
```
31+
32+
— bei übersetzten Modellen zuzüglich eines Sprachwechsels je Property.
33+
34+
## Warum der Neuaufbau überhaupt nötig ist
35+
36+
Jedes Widget muss die Eingaben **aller** Felder sehen, nicht nur die eigene:
37+
Anzeigebedingungen und abhängige Auswahlfelder werden gegen die übrigen Properties
38+
ausgewertet. Deshalb wird das Modell aus dem gesamten Wertebeutel aufgebaut und nicht aus
39+
einem Einzelwert. Das ist richtig — falsch war nur, es **je Widget** zu tun.
40+
41+
## Die Änderung
42+
43+
Die Werte sind für alle Widgets eines Durchlaufs dieselben. Das Modell wird daher **einmal**
44+
gebaut und zwischengespeichert (`modelWithInput()`); der Schlüssel deckt den Inhalt des
45+
Beutels ab, ein geänderter Wert baut neu.
46+
47+
Jeder Aufrufer erhält weiterhin eine **eigene Instanz** — `cloneModel()` gibt einen Klon des
48+
zwischengespeicherten Modells zurück. Das ist gefahrlos, weil `MetaModels\DcGeneral\Data\Model::__clone()`
49+
das Item per `copy()` tief kopiert; ein Listener am `BuildWidgetEvent` kann den Cache also
50+
nicht verunreinigen. `DefaultModel::__clone()` verwirft die Id absichtlich, deshalb setzt
51+
`cloneModel()` sie wieder.
52+
53+
**Die Semantik bleibt unverändert:** Jedes Widget sieht denselben vollständig befüllten
54+
Modellzustand wie zuvor.
55+
56+
## Messung
57+
58+
Speichern von `mm_employees::11` — 27 Widgets, unübersetztes Modell, Symfony-**dev**-Modus,
59+
Xdebug **aus**. Zähler über temporäre Instrumentierung an den Konvertierungsstellen, Wandzeit
60+
des POST über den Browser.
61+
62+
| | vorher | nachher | Faktor |
63+
|---|---:|---:|---:|
64+
| `valueToWidget` | 1.785 | **221** | 8,1 |
65+
| `setProperty` | 1.349 | **99** | 13,6 |
66+
| `widgetToValue` | 1.325 | **75** | 17,7 |
67+
| `encodeValue` | 1.323 | **73** | 18,1 |
68+
| `getWidget` | 76 | 76 | — |
69+
| Wandzeit POST | ≈ 20.000 ms | **≈ 9.500 ms** | ≈ 2,1 |
70+
71+
Die Aufrufzahlen sind deterministisch und unabhängig von der Laufzeitumgebung; die Wandzeit
72+
ist es nicht — im dev-Modus liegt sie höher als in Produktion. Für den Vergleich zählt, dass
73+
beide Messungen unter denselben Bedingungen liefen.
74+
75+
Dass die Wandzeit „nur" um den Faktor 2 sinkt, während die Konvertierungen um Faktor 8 bis 18
76+
fallen, heißt: Der Rest der Zeit steckt woanders — die 76 `getWidget`-Aufrufe für 27 Properties
77+
deuten auf mehrfache Durchläufe der Maske (Validierung, Neuaufbau, Rendern der Antwort).
78+
79+
**Nebenbefund:** Xdebug war *nicht* die Ursache der Langsamkeit. Mit `xdebug.mode=debug` und
80+
`start_with_request=yes` lagen die Zeiten bei 22 s, ohne Xdebug bei 18–23 s — kein
81+
nennenswerter Unterschied. Die Vermutung, der fehlschlagende Debugger-Verbindungsaufbau koste
82+
spürbar, hat sich nicht bestätigt.
83+
84+
## Was offen bleibt
85+
86+
1. **Die drei Konvertierungen je `setProperty`** in MetaModels. Der Selbstvergleich
87+
(`$varValue !== $this->getProperty(...)`) und die anschließende `DifferentValuesException::compare`
88+
rufen beide `valueToWidget` auf. Ein einmaliges Zwischenspeichern des konvertierten Werts
89+
innerhalb von `setProperty` würde zwei davon sparen. Liegt in `metamodels/core`, nicht hier.
90+
2. **Die 76 `getWidget`-Aufrufe.** Zu klären, welche Durchläufe das sind und ob sich einer
91+
davon einsparen lässt.
92+
3. **Der Sprachwechsel je Property** bei übersetzten Modellen — noch nicht gemessen. Ein
93+
Benchmark an einem übersetzten Modell fehlt.
94+
95+
## Messung wiederholen
96+
97+
`.playwrite/bench-save.js` misst die Wandzeit eines Speichervorgangs:
98+
99+
```bash
100+
cd .playwrite
101+
BENCH_URL='/contao/metamodel/mm_employees?act=edit&id=mm_employees::11' node bench-save.js
102+
```
103+
104+
Für die Aufrufzahlen war eine temporäre Instrumentierung nötig (statische Zähler an
105+
`Model::getProperty`/`setProperty` und an `getWidget`/`encodeValue`, Ausgabe per
106+
`register_shutdown_function`). Sie ist nach der Messung wieder entfernt worden — für eine
107+
Wiederholung muss sie neu gesetzt werden.
108+
109+
**Beim Messen beachten:** `verify-dnd.js` sortiert `tl_metamodel_dcasetting` per Drag & Drop
110+
um und verändert damit dauerhaft die Reihenfolge der Eingabemaske — nach einem Lauf steht die
111+
Legende woanders. Wer Messreihen fährt, sollte das Skript aus der Runde nehmen.

‎src/Contao/View/Contao2BackendView/ContaoWidgetManager.php‎

Lines changed: 103 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,20 @@ class ContaoWidgetManager
9999
* @param EnvironmentInterface $environment The environment in use.
100100
* @param ModelInterface $model The model for which widgets shall be generated.
101101
*/
102+
/**
103+
* The model carrying the input values of the current run, see modelWithInput().
104+
*
105+
* @var ModelInterface|null
106+
*/
107+
private ?ModelInterface $inputModel = null;
108+
109+
/**
110+
* The key the cached input model was built for.
111+
*
112+
* @var string|null
113+
*/
114+
private ?string $inputModelKey = null;
115+
102116
public function __construct(EnvironmentInterface $environment, ModelInterface $model)
103117
{
104118
$this->environment = $environment;
@@ -323,26 +337,9 @@ public function getWidget($property, ?PropertyValueBagInterface $inputValues = n
323337
);
324338
}
325339

326-
$model = clone $this->model;
327-
$model->setId($this->model->getId());
328-
329-
if ($inputValues) {
330-
$controller = $environment->getController();
331-
assert($controller instanceof ControllerInterface);
332-
333-
$values = new PropertyValueBag();
334-
foreach ($inputValues->getIterator() as $propertyName => $propertyValue) {
335-
try {
336-
$values->setPropertyValue(
337-
$propertyName,
338-
$this->encodeValue($propertyName, $propertyValue, $inputValues)
339-
);
340-
} catch (\Exception $e) {
341-
}
342-
}
343-
344-
$controller->updateModelFromPropertyBag($model, $values);
345-
}
340+
$model = null === $inputValues
341+
? $this->cloneModel($this->model)
342+
: $this->cloneModel($this->modelWithInput($inputValues));
346343

347344
$event = new BuildWidgetEvent($environment, $model, $propertyDefinitions->getProperty($property));
348345

@@ -354,6 +351,92 @@ public function getWidget($property, ?PropertyValueBagInterface $inputValues = n
354351
return $event->getWidget();
355352
}
356353

354+
/**
355+
* Build the model that carries all passed input values - once per set of values.
356+
*
357+
* Every widget has to see the input of *all* fields, not just its own: display conditions and
358+
* dependent selects are evaluated against the other properties. That is why the model is rebuilt
359+
* from the whole bag rather than from a single value.
360+
*
361+
* Doing so per widget made the work quadratic. getWidget() is called once per property, and each
362+
* call encoded every value and wrote it to a fresh clone - with n properties that is n² encode
363+
* events and n² setProperty calls, and every setProperty converts the value again. Measured on a
364+
* mask with 27 widgets: 1323 encode calls and 1349 setProperty calls for one save.
365+
*
366+
* The values are the same for all widgets of one run, so the model is built once and cached. The
367+
* cache key covers the contents of the bag, so a changed value rebuilds it.
368+
*
369+
* @param PropertyValueBagInterface $inputValues The input values.
370+
*
371+
* @return ModelInterface
372+
*/
373+
private function modelWithInput(PropertyValueBagInterface $inputValues): ModelInterface
374+
{
375+
$key = $this->inputCacheKey($inputValues);
376+
if (null !== $key && null !== $this->inputModel && $key === $this->inputModelKey) {
377+
return $this->inputModel;
378+
}
379+
380+
$environment = $this->getEnvironment();
381+
$controller = $environment->getController();
382+
assert($controller instanceof ControllerInterface);
383+
384+
$model = $this->cloneModel($this->model);
385+
386+
$values = new PropertyValueBag();
387+
foreach ($inputValues->getIterator() as $propertyName => $propertyValue) {
388+
try {
389+
$values->setPropertyValue(
390+
$propertyName,
391+
$this->encodeValue($propertyName, $propertyValue, $inputValues)
392+
);
393+
} catch (\Exception $e) {
394+
// A value that cannot be encoded is left out of the model, exactly as before this
395+
// was extracted - the widget then falls back to the stored value.
396+
continue;
397+
}
398+
}
399+
400+
$controller->updateModelFromPropertyBag($model, $values);
401+
402+
$this->inputModel = $model;
403+
$this->inputModelKey = $key;
404+
405+
return $model;
406+
}
407+
408+
/**
409+
* Clone a model and keep its id - DefaultModel::__clone() drops it on purpose.
410+
*
411+
* @param ModelInterface $model The model to copy.
412+
*
413+
* @return ModelInterface
414+
*/
415+
private function cloneModel(ModelInterface $model): ModelInterface
416+
{
417+
$copy = clone $model;
418+
$copy->setId($model->getId());
419+
420+
return $copy;
421+
}
422+
423+
/**
424+
* Build a cache key over the contents of the value bag, or null when they cannot be hashed.
425+
*
426+
* @param PropertyValueBagInterface $inputValues The input values.
427+
*
428+
* @return string|null
429+
*/
430+
private function inputCacheKey(PropertyValueBagInterface $inputValues): ?string
431+
{
432+
try {
433+
return \md5(\serialize($inputValues->getArrayCopy()));
434+
} catch (\Throwable) {
435+
// Not hashable - fall back to rebuilding, which is what happened before anyway.
436+
return null;
437+
}
438+
}
439+
357440
/**
358441
* Build the date picker string.
359442
*

0 commit comments

Comments
 (0)