Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | doctrine-2151829.patch | 16.42 KB | dawehner |
| #21 | doctrine-2151829.patch | 58.74 KB | dawehner |
| #7 | doctrine_regexp.patch | 988 bytes | chx |
| #2 | xhprof.tar_.gz | 32.33 MB | dawehner |
Comments
Comment #1
webchickComment #2
dawehnerInteresting, 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.phpFor 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?
Comment #3
webchickHaha. 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.
Comment #4
chx CreditAttribution: chx commentedSince #1901390: Enable StaticReflectionParser performance optimization to speed up Annotation parsing it is supposed to do just that?
Comment #5
chx CreditAttribution: chx commented*cough* this probably needs to be submitted upstream; tested; yadda yadda. I can't find the time. The problem is
class foo extends baris not found by the regexp it only findsclass foo {and that's plain wrong.Comment #7
chx CreditAttribution: chx commentedComment #8
chx CreditAttribution: chx commentedComment #9
chx CreditAttribution: chx commentedI opened an issue at https://github.com/doctrine/common/issues/307 but that's as far as I can take this.
Comment #10
dawehnerI try to work on that one.
Comment #11
chx CreditAttribution: chx commentedhttps://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.
Comment #12
garphy CreditAttribution: garphy commentedFWIW, 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.
Comment #13
olli CreditAttribution: olli commentedFor 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
Comment #14
catchI'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?
Comment #15
webchick+100 to me.
Comment #16
tim.plunkett#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.
Comment #17
chx CreditAttribution: chx commentedYeah, interfaces have no annotations, so I am not sure why is that on the list.
Comment #18
olli CreditAttribution: olli commentedMaybe we could move that interface to another namespace/folder?
Comment #19
chx CreditAttribution: chx commentedInterfaces are completely off topic in this issue: they have no annotations and they are not read by this parser.
Comment #20
olli CreditAttribution: olli commentedFiled #2156541: Move interface out of plugin folders for the interface.
Comment #21
dawehnerThis patches adds the temporary fork to the composer.json file and updates that dependency.
Comment #22
chx CreditAttribution: chx commentedWe do not use Common much so the version bump is meaningless to us.
Comment #23
webchickYAY 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?
Comment #24
dawehnerLinking to the other issue: #2157045: Update doctrine/common to 2.5-dev
Comment #25
dawehnerRerolled the latest patch.
Comment #26
chx CreditAttribution: chx commentedThat still looks good.
Comment #27
webchickCool. 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!
Comment #28
tim.plunkettThis was not pushed. Last commit was for doctrine/common 2.5
Comment #29
webchickOk, opened up #2157189: [Policy, no patch] How to handle upstream fixes that don't get committed fast enough to discuss.
And, ahem, pushed. ;P
Comment #31
catchWe're running dev, but we're not forked. Untagging.