Skip to content

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

New issue

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

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

Already on GitHub? Sign in to your account

Closed

Conversation

Kocal
Copy link
Member

@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
Member 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants