Follow-up to #2421451: Drupal needs comments in opcache

Problem/Motivation

SimpleAnnotationReader loads classes while parsing them. This puts more things in the opcache than we would like.

Doctrine has essentially "won't fixed" this bug, since they're planning to deprecate SimpleAnnotationReader.

AnnotationReader itself both loads classes and uses reflection, this is much too memory heavy for Drupal given the number of plugins in core, and is the reason we contributed to SimpleAnnotationReader in the first place.

Doctrine's support cycle is out of sync with ours, since they already require PHP 5.6.

Proposed resolution

We need our own annotation parser, as a component, which uses the PHP tokenizer rather than reflection and doesn't load any classes as PHP. The subtree split will allow other projects to use that if they wish.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

cilefen created an issue. See original summary.

cilefen’s picture

Issue summary: View changes
dawehner’s picture

Yeah, sadly DocParser is even final, it will be hard to override anything without reimplementing basically everything.

neclimdul’s picture

Additionally, while trying to resolve a performance issue, Doctrine basically told us SimpleAnnotationReader should never be used and is deprecated. So we're already in need of replacing our use of SimpleAnnotationReader and writing our own.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +neworleans2016, +Needs issue summary update, +Triaged for D8 major current state

@stephanie_42 and I are triaging this Major issue at New Orleans.

According to the upstream issue mentioned in #4, it seems AnnotationReader is the way to go. But #4, and other commentators on the parent issue also mention writing our own. I don't understand annotation parsing enough right now to comment on rolling our own. I'm tagging "Needs issue summary update" because we need a plan.

xjm’s picture

Thanks @cilefen for helping triage this! Updating issue credit for the triage work. @stephanie_42, if you are following, can you also comment to ensure you get credit for the triage work?

catch’s picture

Discussed this with @alexpott @effulgentsia @xjm @webchick @cottser and we agreed this is definitely major.

APC used to get fragmented and then spit php warnings when it couldn't find a big enough spot for a file.

opcode definitely empties itself when it gets full, not sure how it handles fragmentation.

Both of these are 'very bad' and it's nearly impossible for someone running into them to trace the problem back to this issue, unless they already know about it. You'd have to read through the files list on opcode.php and spot that there are annotated classes from enabled modules but which your site never uses to have any idea at all.

The issue on cache rebuilds about OOM is also very hard to debug - there aren't good memory tools (including xhprof) that can tell you what exactly is responsible for using memory. However loading PHP classes is memory that can never be reclaimed by gc, so while it might not be the worst thing we do, it's increases the baseline of memory usage in a way that can't be tweaked except by fixing this.

I think we need a new issue to discuss how to deal with the problem of Doctrine deprecating SimpleAnnotationParser/slating it for removal as well as the final which blocks this. We added that as a pre-requisite of using annotations in the first place, so may need to either find another library or adopt that code as our own (even if via a new library or fork somehow).

Not opening that issue yet, but when I do, will postpone this one on it.

xjm’s picture

Issue tags: +Triaged D8 major

Tagging. Thanks for helping confirm this major.

Stephanie_42’s picture

Thank you @cilefen and @xjm for your guidance and emails.

Per the upstream issue in #4 and cilefen's comment in #6 - Doctrine says that "AnnotationReader" is the correct successor to "SimpleAnnotationReader" and is seems that the comments in the parent issue (#4) are moving in that direction.

xjm’s picture

(Updating credit, thanks @Stephanie_42!)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Title: Doctrine Annotations reader loads classes while parsing them » Remove dependency on Doctrine annotation reader by writing our own solution
Component: system.module » base system
Issue summary: View changes
Issue tags:

Doctrine (along with Zend framework) now requires PHP 5.6. Since for at least the next 6 months we'll be supporting PHP 5.5, their support schedule is out of sync with ours.

Additionally this issue shows that:

- there's a major bug with Doctrine simple annotation reader (i.e. it does the exact thing that chx's contribution to it was designed to prevent)

- Doctrine has no intention of fixing that bug, and in fact is planning to deprecate it altogether.

With the combination of these, it's time to discuss seriously replacing it altogether with our own component. Doing a small title and issue summary update but it could do with fleshing out of course.

catch’s picture

Issue summary: View changes
Issue tags:
Mile23’s picture

So the difference between this issue and the solution in #2557433-17: Verify that the plugin annotation system does not try to resolve @see and other annotations is that this issue specifies that we shouldn't use reflection, or otherwise class-load the files we're parsing.

That means we'll accomplish the goals of #2557433: Verify that the plugin annotation system does not try to resolve @see and other annotations which is to avoid trying to autoload classes for annotations like @see.

Is there any reason not to use https://github.com/php-annotations/php-annotations/blob/master/src/annot... or another external library?

dawehner’s picture

Is there any reason not to use https://github.com/php-annotations/php-annotations/blob/master/src/annot... or another external library?

I think there is no reason not to use a library. The particular example you have chosen: https://php-annotations.readthedocs.io/en/latest/UsingAnnotations.html#a... seems though to have a different syntax. I guess we need to be 100% compliant with what we have right now.

Mile23’s picture

Status: Active » Postponed

Classes like Component\AnnotatedClassDiscovery need more tests before this can go: #2435607: Tests for Annotation Component

dawehner’s picture

+1 for improving the test coverage.

Replacing that would actually let us probably keep AnnotatedClassDiscovery basically exactly the same. Instead it would "just" replace the annotation parser, right?

Mile23’s picture

That's what it looks like. Basically, we only use reflection so we can feed doctrine's SimpleAnnotationReader, which then only feeds the strings to doctrine's parser/lexer. So we should replace SimpleAnnotationReader with our own semi-clone that parses tokens.

I started to write the token parser, but realized I couldn't even let the testbot figure out if I did the right thing. :-)