Skip to content

Performance issue with the "lint:xliff" command #27564

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.

Sign up for GitHub

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

Jump to bottom
Closed
javiereguiluz opened this issue Jun 9, 2018 · 7 comments
Closed

Performance issue with the "lint:xliff" command #27564

javiereguiluz opened this issue Jun 9, 2018 · 7 comments
Assignees
@javiereguiluz
Labels
Bug Performance Status: Needs Review Translation

Comments

@javiereguiluz
Copy link
Member

javiereguiluz commented Jun 9, 2018 •
edited
Loading

Symfony version(s) affected: all

Description
The lint:xliff command is too slow.

How to reproduce
I put the following on my Travis CI config:

$ ./tests/bin/console lint:xliff "src/Resources/translations"

But the build runs out of time (see full details):

No output has been received in the last 10m0s, this potentially indicates a
stalled build or something wrong with the build itself.

The build has been terminated.

Additional context
I've run this command locally linting just 1 file and it was extremely slow. I share the Blackfire profile:

$ blackfire run ./tests/bin/console lint:xliff src/Resources/translations/EasyAdminBundle.en.xlf

Profile:

https://blackfire.io/profiles/5f1be513-3f01-414d-8759-11ae84a00ce3/graph

The problem seems to be in PHP's DOMDocument::schemaValidate but maybe we are doing something that makes it slow?
@javiereguiluz javiereguiluz mentioned this issue Jun 9, 2018
Merged
javiereguiluz added a commit to EasyCorp/EasyAdminBundle that referenced this issue Jun 9, 2018
@javiereguiluz
minor #2254 Disable the linting of XLIFF files (javiereguiluz)
b93e88b 
This PR was merged into the 2.0.x-dev branch.

Discussion
----------

Disable the linting of XLIFF files

More details at this issue: symfony/symfony#27564

Commits
-------

0575f17 Disable the linting of XLIFF files
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 9, 2018 •
edited
Loading

No output has been received in the last 10m0s

if we cannot make the function faster, we could at least output some progress bar so that Travis doesn't think we're frozen.

@stof
Copy link
Member

stof commented Jun 15, 2018

The problem seems to be in PHP's DOMDocument::schemaValidate but maybe we are doing something that makes it slow?

next step would be to profile the C code then.

@ZHB
Copy link

ZHB commented Jun 20, 2018

It appears that the W3C intentionally imposes a delay on the loading of DTDs, as seen here : https://www.w3.org/Help/Webmaster

The W3C servers are slow to return DTDs. Is the delay intentional?
Yes. Due to various software systems downloading DTDs from our site millions of times a day (despite the caching directives of our servers), we have started to serve DTDs and schema (DTD, XSD, ENT, MOD, etc.) from our site with an artificial delay. Our goals in doing so are to bring more attention to our ongoing issues with excessive DTD traffic, and to protect the stability and response time of the rest of our site. We recommend HTTP caching or catalog files to improve performance.

We can improve performances by loading http://www.w3.org/2001/xml.xsd in local file system with libxml_set_external_entity_loader().

@stof
Copy link
Member

stof commented Jun 20, 2018

OK, found the issue there. The XliffFileLoader has some special logic to use a local copy of the xml schema when validating the schema. But the XliffLintCommand does not use that logic (and so uses the remote copy). We should use the same logic in both cases (we could extract the getSchema implementation to an internal utility reused by both classes)

@stof
Copy link
Member

stof commented Jun 20, 2018

Btw, the XliffLintCommand is also missing support for xliff 2.0 (unlike the loader). It does not select the schema.

@javiereguiluz javiereguiluz self-assigned this Jun 20, 2018
@stof
Copy link
Member

stof commented Jun 20, 2018

Suggestion: extract a Xliff (internal) utility, which would allow reusing both the version detection and the schema processing

@javiereguiluz
Copy link
Member Author

@ZHB and @stof thanks for this great detective work! The problem is clear now. I'll try to send a fix based on Christophe suggestions. Thanks!

@javiereguiluz javiereguiluz mentioned this issue Jun 20, 2018
Merged
@fabpot fabpot closed this as completed Jun 20, 2018
fabpot added a commit that referenced this issue Jun 20, 2018
@fabpot
feature #27653 [Translation] Improved the performance of the lint:xli…
1ff80e3 
…ff command (javiereguiluz)

This PR was squashed before being merged into the 4.2-dev branch (closes #27653).

Discussion
----------

[Translation] Improved the performance of the lint:xliff command

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27564
| License       | MIT
| Doc PR        | -

As suggested by @stof I extracted the schema validation logic from XliffFileLoader to reuse it in the `lint:xliff` command. The validation is now instantaneous, so the command is blazing fast!

Commits
-------

e53bf58 [Translation] Improved the performance of the lint:xliff command
@nicolas-grekas nicolas-grekas mentioned this issue Aug 7, 2018
Merged
nicolas-grekas added a commit that referenced this issue Aug 8, 2018
@nicolas-grekas
bug #28152 [Translation] fix perf of lint:xliff command (nicolas-grekas)
9fae8f4 
This PR was merged into the 4.1 branch.

Discussion
----------

[Translation] fix perf of lint:xliff command

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27564
| License       | MIT
| Doc PR        | -

#27653 has been merged on master as an improvement, but the perf issue is a killer.
Our CI spends 1 minutes on just a few translation test cases.
Only 4.1 has this behavior. That's a bug.

Commits
-------

02c69b1 [Translation] fix perf of lint:xliff command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@javiereguiluz javiereguiluz

Labels
Bug Performance Status: Needs Review Translation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @javiereguiluz @ZHB @nicolas-grekas @stof @carsonbot