Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Map] Improve backend performance when rendering HTML "view" attribute #2178

Closed

Conversation

Kocal
Copy link
Collaborator

@Kocal Kocal commented Sep 18, 2024

Q A
Bug fix? no
New feature? no
Issues Fix #...
License MIT

My intuition was right, I knew there was some back optimization to be done on Map :)

For the context, I've created a Map with 1000 Marker and InfoWindow (it represents the first 1000 cities from France in alphabetical order, but it can also be a store-locator, etc...)
Capture d’écran 2024-09-18 à 12 22 40

The code looks like:

$map = new Map();
$map->center(new Point(48.8566, 2.3522));
$map->zoom(6);

$cities = require_once __DIR__.'/../../config/cities.php';

foreach ($cities as $i => $city) {
    $map->addMarker(
        new Marker(
            position: new Point($city['latitude'], $city['longitude']),
            title: $city['label'],
            infoWindow: new InfoWindow(
                headerContent: '<b>'.$city['label'].'</b>',
                content: 'Welcome in '.$city['label'],
                opened: $i === 300,
            ),
        )
    );
}

Before

Before this PR, when profiling the page with Blackfire, we can see that StimulusAttributes#toString() takes ~62% of the response time (~222ms), because of the Twig escaping code (preg_replace_callback and ord()):
Capture d’écran 2024-09-18 à 12 26 37

This is long because our view attribute can be very big, it contains a lot of data. But, since it's an array with a known structure, etc..., maybe we can... not use Twig escaping strategy?

Note
I will open an issue on Twig, maybe it already exists, but it may be interesting to see if we can improve things at the root.

EDIT: opened twigphp/Twig#4322

Now

I've removed view' => $map->toArray() from the StimulusAttributes, and added 'data-symfony--ux-'.$this->getName().'-map--map-view-value="'.htmlentities(json_encode($map->toArray(), flags: JSON_THROW_ON_ERROR)).'"' at the final rendering place.

When profiling the page with Blackfire, I can see an improvment of ~63% on the response time 🚀 :
image

I'm not 100% sure, but I believe htmlentities and json_encode are enough for our usecase? I mean, it still works, and HTML in InfoWindow still works too:
Capture d’écran 2024-09-18 à 12 33 28

@carsonbot carsonbot added Map Status: Needs Review Needs to be reviewed labels Sep 18, 2024
@Kocal Kocal force-pushed the map-increase-backend-rendering-performances branch 3 times, most recently from 03eebb1 to b77e7b2 Compare September 18, 2024 10:50
@Kocal
Copy link
Collaborator Author

Kocal commented Sep 18, 2024

Closing in favor of a new PR :)

javiereguiluz added a commit that referenced this pull request Sep 24, 2024
…erformances by switching to `html` escaping strategy (Kocal)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[StimulusBundle] Improve `StimulusAttributes` rendering performances by switching to `html` escaping strategy

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - For new features, provide some code snippets to help understand usage.
 - Features and deprecations must be submitted against branch main.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

A better alternative to #2178, by changing the escaping strategy for HTML attributes value from `html_attr` to `html`, as indicated by `@stof` in twigphp/Twig#4322 (comment).

On my use case (a `Map` with 1000 `Marker` and `InfoWindow`), [I have a performance gain of ~68%](https://blackfire.io/profiles/compare/56f4d7d2-ee56-487f-8a78-420f4037165f/graph):
<img width="1057" alt="image" src="https://github.com/user-attachments/assets/8b7ff48d-8e6e-4dae-b09e-2c323bc2449d">

This PR should also improve performances of our packages using the `StimulusAttributes` DTO, like Chart.js, LiveComponent, ...

---

> [!IMPORTANT]
> The initial PR changed a bit, the default rendering strategy does not change, but instead we introduce a new configuration to use the new (and optimized) rendering strategy, see  #2180 (comment).

Commits
-------

647532a [StimulusBundle] Improve `StimulusAttributes` rendering performances by switching to `html` escaping strategy
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants