Trying to install D8 on DreamHost. This is successfully running webchick.net on D7.

Got to approximately 98-99% (after about literally 7-8 minutes) then dies at the end with:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: http://webchick.net/gs/core/install.php?langcode=en&profile=standard&id=1&op=do_nojs&op=do
StatusText: OK
ResponseText:
Fatal error:  Maximum execution time of 30 seconds exceeded in /home/snarkles/d7-git.webchick.net/gs/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/TokenParser.php on line 53

Before I upped my local drush.ini file to 256M of memory, it was also dying in a similar fashion due to out of memory issues on drush si. That started happening maybe ~1 month ago.

Something smelly is up here, and we need to fix it. We bled our eyeballs out trying to get D7 to install in 32M. 256 is sheer insanity.

The current suspect is the slightly broken optimization regexp in StaticReflectionParser::parse() as it causes Doctrine to tokenize whole classes instead of file headers. It only picks up classes that do not extend another class or implement an interface and let's face it: that's rare.

Files: 
CommentFileSizeAuthor
#25 doctrine-2151829.patch16.42 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,763 pass(es).
[ View ]
#21 doctrine-2151829.patch58.74 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,617 pass(es).
[ View ]
#7 doctrine_regexp.patch988 byteschx
PASSED: [[SimpleTest]]: [MySQL] 59,291 pass(es).
[ View ]
#2 xhprof.tar_.gz32.33 MBdawehner

Comments

webchick’s picture

Issue summary:View changes
dawehner’s picture

StatusFileSize
new32.33 MB

Interesting, I tried to reproduce locally by writing some xhprof data at every install batch request. Sadly this slowed down the batch request, so that there has been more requests as usual,
so that the average memory is lower. (There is a bunch of xhprof files/screenshots

If you sort by incl memory usage you will see Doctrine\Common\Annotations\TokenParser::__constructto use up to 600MB, which calls http://php.net/manual/en/function.token-get-all.php

For a long file, this can be quite a big amount of values, so I wonder whether we could patch that file in doctrine to not load all tokens but just all tokens until the first appearance of "class",
as from my understanding, this file does not need more information.

Do you know whether dreamhost is using 5.4 or 5.3?

webchick’s picture

Haha. Looks like 5.3.27. What the eff? How did that even install? Oh. I guess cos #1498574: [policy, no patch] Require PHP 5.4 is not in yet.

Localhost is 5.4.8, though.

chx’s picture

to not load all tokens but just all tokens until the first appearance of "class",

Since #1901390: Enable StaticReflectionParser performance optimization to speed up Annotation parsing it is supposed to do just that?

chx’s picture

Status:Active» Needs review
StatusFileSize
new900 bytes
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

*cough* this probably needs to be submitted upstream; tested; yadda yadda. I can't find the time. The problem is class foo extends bar is not found by the regexp it only finds class foo { and that's plain wrong.

Status:Needs review» Needs work

The last submitted patch, 5: doctrine_regexp.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new988 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,291 pass(es).
[ View ]
chx’s picture

Issue summary:View changes
chx’s picture

I opened an issue at https://github.com/doctrine/common/issues/307 but that's as far as I can take this.

dawehner’s picture

Assigned:Unassigned» dawehner

I try to work on that one.

chx’s picture

https://github.com/doctrine/common/pull/308 it's here. we need this asap. Can we just commit it to core and bother with upstream when they get it? Do we need to put up a fork of Doctrine Commons in a Drupal.org sandbox somewhere or can we use dawehner's from github? We need this stat.

garphy’s picture

FWIW, patch in #7 let me finish my first graphical install of D8 \o/ (i was forced to use drush only 'til now).
Congrats for spotting the bug.

olli’s picture

For me #7 makes drush si standard 20% faster.

Found few files that does not match the regex:
core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayRouterInterface.php
core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldWidget/DatetimeDatelistWidget.php
core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldWidget/DatetimeDefaultWidget.php
core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldFormatter/DatetimeDefaultFormatter.php
core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldFormatter/DatetimePlainFormatter.php

catch’s picture

I'm OK with committing this as a local patch as long as we open a critical follow-up to

either 1. update Doctrine as soon as it's merged 2. put it in a proper fork in composer if for some reason it's not merged relatively quickly
AND 3. ensure there's some test coverage so that if either #1 or #2 go wrong at some point it doesn't break again.

Sounds OK?

webchick’s picture

+100 to me.

tim.plunkett’s picture

#13, the 4 Datetime classes aren't picked up because the filenames don't match the classnames (DateTime vs Datetime).
Not sure why an interface would matter.

EDIT: Opened #2156283: Fix case of Datetime widgets and formatters to fix those four.

chx’s picture

Yeah, interfaces have no annotations, so I am not sure why is that on the list.

olli’s picture

Maybe we could move that interface to another namespace/folder?

chx’s picture

Interfaces are completely off topic in this issue: they have no annotations and they are not read by this parser.

olli’s picture

dawehner’s picture

StatusFileSize
new58.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,617 pass(es).
[ View ]

This patches adds the temporary fork to the composer.json file and updates that dependency.

chx’s picture

Status:Needs review» Reviewed & tested by the community

We do not use Common much so the version bump is meaningless to us.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

YAY for this bring RTBC. However, now that I look at the actual patch...

1) It is HUGE. :( Apart from the Composer stuff, I would've expected this to only have a single line diff similar to the original patch. Since we are from now on going to be responsible for maintaining this code, I'd love to see the update to Doctrine 2.5.0-dev as a separate issue/patch (which can be committed as soon as it's back from testbot, basically).

2a) As much as I trust dawehner and everything, I'm not real crazy about the idea of Drupal core code being pulled from non-official, personal, off-site repos. I think if we're going to make it a habit of forking upstream libraries when they don't move fast enough for us, we should have an "official" repo here on D.o with the same commit access as Drupal core and pull from that instead. I can circle the wagons with the other core committers and figure out a process for this, but I'd need some help with the git-fu required to pull in Daniel's branch.

2b) Or, alternately, can we sub-class something or other in our own code so that we don't need to fork Doctrine at all?

dawehner’s picture

Linking to the other issue: #2157045: Update doctrine/common to 2.5-dev

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new16.42 KB
PASSED: [[SimpleTest]]: [MySQL] 59,763 pass(es).
[ View ]

Rerolled the latest patch.

chx’s picture

Status:Needs review» Reviewed & tested by the community

That still looks good.

webchick’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+revisit before beta

Cool. Tried this just now and 8.x's standard profile once again installs comfortably in 128M! When I do trigger an OOM error at a lower level of memory_limit (the lowest I could get it to install with is 80M which is still more than twice that of D7), the error messages are coming from other parts of the system, not this one.

So while there may be more things to optimize during the installer yet (the plugin system and Composer's autoloader might be worth a look next to see if there are obvious things to fix there as well), this definitely made the situation HUGELY better. Thanks so much for all of your great work! :D

Per catch's #14, the decision we made to go with here was a "proper" fork in Composer.js which contains just the contents of the upstream PR at https://github.com/doctrine/common/issues/307 (the rest of the "catching up with doctrine" update was done in #2157045: Update doctrine/common to 2.5-dev), which includes test coverage for this change.

So I think we are good to go! I do want to discuss separately if linking to individual developers' sandboxes on Github is a good way to go in Composer.js for these sorts of (hopefully temporary) forks, but there's precedent for having done this already with PHPUnit (can't find the issue off-hand but Daniel shared it in IRC), so this just sticks with status quo of what we've already done in other places. I'll open a follow-up for that.

So, committed and pushed to 8.x. Tagging as "revisit before release" to ensure we remove this and merge back in with upstream at a later date. Great work!

tim.plunkett’s picture

This was not pushed. Last commit was for doctrine/common 2.5

webchick’s picture

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

catch’s picture

Issue tags:-revisit before beta

We're running dev, but we're not forked. Untagging.