Skip to content
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

Add notes about Doctrine performances #107

Merged
merged 1 commit into from
Oct 10, 2016

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented Sep 7, 2016


### Search Filter

When using the `SearchFilter` and case insensivity, Doctrine will be using the `LOWER` sql function. Depending on your driver, you may want to carefully index it by using a [*function-based index*](http://use-the-index-luke.com/sql/where-clause/functions/case-insensitive-search) or it will impact performances with a huge collection.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doctrine will use ... SQL

Also, we should probably note that function-based index alone is not enough for good performance. When using PostgreSQL for example, it's necessary to add a text_pattern_ops index, and also gin_trgm_ops (from pg_trgm). But perhaps it's out of scope... 😛

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.:

CREATE INDEX idx_places_name_trgm ON places USING GIN (lower(name) gin_trgm_ops);
CREATE INDEX idx_places_name_pattern ON places (lower(name) text_pattern_ops);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's out of scope (because it's Driver-dependent) but I totaly agree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soyuka soyuka force-pushed the soyuka-patch-1 branch 2 times, most recently from 527d43e to b0e7d7f Compare September 8, 2016 14:59
@soyuka
Copy link
Member Author

soyuka commented Sep 8, 2016

Comments addressed thx @teohhanhui

Even though we're selecting only partial results (serialized properties) with Doctrine, it'll try to hydrate some relations with lazy joins (for example `OneToOne` relations). It's recommended to take a look at the Symfony Profiler, check what the generated SQL queries are doing in background and see if those may impact performance.

To force Doctrine to only hydrate partial values you need to use the [`Query::HINT_FORCE_PARTIAL_LOAD`](http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/dql-doctrine-query-language.html#query-hints).
Be careful, using this query hint will force the use of partial selects. Some properties might not be available even if you expect them. If you want to be sure that Doctrine fetches them, use Eager joins and make sure that properties are serializable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New paragraph? Missing line break.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's linked to the previous one, I'd keep it like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want a newline without starting a new paragraph, you need to add two trailing spaces on the previous line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving it like this is really confusing...

@soyuka
Copy link
Member Author

soyuka commented Sep 8, 2016

I think I have to wrap lines manually? What's the limit?

@teohhanhui
Copy link
Contributor

Yes, it's better to use hard line wrapping in Markdown (it's designed with that in mind after all). Currently I think we're going for 120 characters.

@dunglas
Copy link
Member

dunglas commented Sep 21, 2016

@soyuka is this PR ready for merging?

@soyuka
Copy link
Member Author

soyuka commented Sep 21, 2016

yes

@@ -23,7 +23,7 @@ The search filter supports `exact`, `partial`, `start`, `end`, and `word_start`
* `word_start` strategy uses `LIKE text% OR LIKE % text%` to search for fields that contains the word starting with `text`.

Prepend the letter `i` to the filter if you want it to be case insensitive. For example `ipartial` or `iexact`. Note that
this will use the `LOWER` function and **will** impact performances if there is no [*function-based index*](http://use-the-index-luke.com/sql/where-clause/functions/case-insensitive-search).
this will use the `LOWER` function and **will** impact performances [if there is no proper index](performance.md#search-filter).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance (uncountable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still this one to fix.


When using the `SearchFilter` and case insensivity, Doctrine will use the `LOWER` SQL function. Depending on your
driver, you may want to carefully index it by using a [function-based
index](http://use-the-index-luke.com/sql/where-clause/functions/case-insensitive-search) or it will impact performances
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance (uncountable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too

@soyuka
Copy link
Member Author

soyuka commented Oct 7, 2016

This one should be good to go

[`PaginationExtension`](https://github.com/api-platform/core/blob/master/src/Bridge/Doctrine/Orm/Extension/PaginationExtension.php)
by setting the query hint:

```php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should include <?php now.


```yaml
# app/config/services.yml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep the services: key to make it copy/pastable.

To alter the `Query` object on an item data provider, we can also create an `QueryHintExtension` which will alter the result:

```php
// src/AppBundle/Doctrine/Orm/Extension/QueryHintExtension.php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should include <?php

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -23,7 +23,7 @@ The search filter supports `exact`, `partial`, `start`, `end`, and `word_start`
* `word_start` strategy uses `LIKE text% OR LIKE % text%` to search for fields that contains the word starting with `text`.

Prepend the letter `i` to the filter if you want it to be case insensitive. For example `ipartial` or `iexact`. Note that
this will use the `LOWER` function and **will** impact performances if there is no [*function-based index*](http://use-the-index-luke.com/sql/where-clause/functions/case-insensitive-search).
this will use the `LOWER` function and **will** impact performances [if there is no proper index](performance.md#search-filter).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still this one to fix.


When using the `SearchFilter` and case insensivity, Doctrine will use the `LOWER` SQL function. Depending on your
driver, you may want to carefully index it by using a [function-based
index](http://use-the-index-luke.com/sql/where-clause/functions/case-insensitive-search) or it will impact performances
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too

$this->managerRegistry = $managerRegistry;
$this->decorated = $decorated;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra blank line

}

/**
* @inheritdoc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{@inheritdoc}



/**
* @inheritdoc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{@inheritdoc}

use Doctrine\ORM\Tools\Pagination\Paginator as DoctrineOrmPaginator;
use Doctrine\ORM\Query;

class QueryHintPaginationExtension implements QueryResultCollectionExtensionInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this class final please?


To force Doctrine to only hydrate partial values you need to use the
[`Query::HINT_FORCE_PARTIAL_LOAD`](http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/dql-doctrine-query-language.html#query-hints).
Be careful, using this query hint will force the use of partial selects. Some properties might not be available even if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space to remove at the start of the line

To force Doctrine to only hydrate partial values you need to use the
[`Query::HINT_FORCE_PARTIAL_LOAD`](http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/dql-doctrine-query-language.html#query-hints).
Be careful, using this query hint will force the use of partial selects. Some properties might not be available even if
you expect them. If you want to be sure that Doctrine fetches them, use Eager joins and make sure that properties are
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eager (no reason to have an uppercase E)

}

/**
* @inheritdoc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{@inheritdoc}

@soyuka
Copy link
Member Author

soyuka commented Oct 7, 2016

thanks, updated

@dunglas dunglas merged commit 58a8fc7 into api-platform:master Oct 10, 2016
@dunglas
Copy link
Member

dunglas commented Oct 10, 2016

Thanks @soyuka

@soyuka
Copy link
Member Author

soyuka commented Oct 10, 2016

thanks @teohhanhui @dunglas for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants