Problem/Motivation

We have a Timer::start() call in DrupalKernel::boot() which is just used by devel module.

Change Record: https://www.drupal.org/node/2489636

Proposed resolution

Remove the call to the timer API in the boot() method

Remaining tasks

create patch
Approve Change Record: https://www.drupal.org/node/2489636

Update change record with information in #28

User interface changes

n/a

API changes

n/a

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it will improve performance.
Issue priority Normal because it is just a clean up of unnecessary functionality. Does not impact any initiative.
Prioritized changes The main goal of this issue is performance.
Disruption Disruptive for devel module but module maintainer is ok with it. See #2 and it has a change record https://www.drupal.org/node/2489636
CommentFileSizeAuthor
#16 interdiff-2474047-4-13.txt501 bytesarturogarrido
#13 drupal-core-remove-timer-from-drupalkernel-2474047-6.patch825 bytesarturogarrido
#9 remove_timer_from-2474047-7.patch824 bytesmikeburrelljr
#7 drupal-core-remove-timer-from-drupalkernel-2474047-6.patch825 bytesarturogarrido
#4 remove_timer_from-2474047-4.patch500 bytesejabrown

Comments

pwolanin’s picture

pwolanin commented

Is there an easy way for Devel to inject a timer?

An alternative - if we start using assert((), the timer call could be wrapped in that.

pwolanin’s picture

pwolanin commented
Issue summary: View changes
Issue tags: -Performance regression +Performance, +Novice

Discussed with Moshe in person and he was ok with removing this call from the boot code.

ejabrown’s picture

ejabrown commented

I'm at the DrupalCon LA sprint and I can work on this issue.

ejabrown’s picture

ejabrown commented
Status: Active » Needs review
StatusFileSize
new remove_timer_from-2474047-4.patch500 bytes
pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

dawehner notes that there is a use statement that cane be removed also

mikeburrelljr’s picture

mikeburrelljr commented

At DC/LA, investigating @pwolanin's request.

arturogarrido’s picture

arturogarrido commented
Assigned: Unassigned » arturogarrido
Status: Needs work » Needs review
StatusFileSize
new drupal-core-remove-timer-from-drupalkernel-2474047-6.patch825 bytes

use statement removed

markie’s picture

markie commented
Assigned: arturogarrido » Unassigned

Un-assigning per mentor suggestion.

mikeburrelljr’s picture

mikeburrelljr commented
StatusFileSize
new remove_timer_from-2474047-7.patch824 bytes

Remove use statement.

Status: Needs review » Needs work

The last submitted patch, 9: remove_timer_from-2474047-7.patch, failed testing.

Status: Needs work » Needs review

mikeburrelljr queued 9: remove_timer_from-2474047-7.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: remove_timer_from-2474047-7.patch, failed testing.

arturogarrido’s picture

arturogarrido commented
Status: Needs work » Needs review
StatusFileSize
new drupal-core-remove-timer-from-drupalkernel-2474047-6.patch825 bytes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work
Issue tags: +Needs change record, +Needs beta evaluation

Change looks solid and it seems to pass the tests so RTBC for me. Before that we need still change record and beta evaluation.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented

I'll review this as well and create change record node

arturogarrido’s picture

arturogarrido commented
StatusFileSize
new interdiff-2474047-4-13.txt501 bytes

Clarification: Patch #13 is same patch as #7. Reason is #9 patch failed so I resubmitted to trigger test request again for my previous patch.

Attaching interdiff-2474047-4-13.txt

Hiding previous patches.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented

Patch applies cleanly and passes all tests.
Create change record: https://www.drupal.org/node/2489636

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Is there known workaround for this? If there is we should include that.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented

Added link to reference for alternative to change record.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -Needs change record

Seems to do the thing. Beta evaluation and we are ready to rock!

arturogarrido’s picture

arturogarrido commented
Issue summary: View changes
Issue tags: -Needs beta evaluation
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs work » Reviewed & tested by the community

Looks good and all the necessary parts are here. Thanks for your great work all!

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented
Issue summary: View changes

Marked change record as approved.

  • xjm committed 17a7f55 on 8.0.x 
    Issue #2474047 by arturogarrido, ejabrown, mikeburrelljr, acrosman,...
xjm’s picture

xjm
she/her
Primary language English
commented

This issue is a prioritized change as per https://www.drupal.org/core/beta-changes and its benefits outweigh any disruption. We've confirmed that the change is okay for the affected contrib projects and that a replacement is available. Committed and pushed to 8.0.x. I also published the CR. Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

 

mikey_p’s picture

mikey_p commented
Status: Fixed » Needs work

I'm not sure this is a good idea. At a minimum, the change record should be updated to note that KernelEvents::REQUEST is MUCH, MUCH later in the request, and could throw off numbers up to 50%.

I added three timers (original boot timer, one in a high priority stack middleware, and one in a normal priority request subscriber) and then logged their output in a KernelEvents::TERMINATE subscriber and here are the difference on a site with a half dozen contrib modules installed:

Front page:

Boot timer: 51.78
Middleware timer: 47.06
Request timer: 25.04

/admin/content

Boot timer: 50.03
Middleware timer: 44.24
Request timer: 21.09

/user/1

Boot timer: 52.76
Middleware timer: 48.01
Request timer: 25.37

/node/add/article

Boot timer: 59.05
Middleware timer: 53.78
Request timer: 28.05

I found this working on the console_logger module for D8, but I'm sure any other module that needs to get accurate response times is going to have problems with this (we're also logging this information to statsd, and now having an accurate timer from initial bootstrap is not possible).

I'm not sure what the best solution is, but at a minimum, we should at least update the change record to suggest using high priority stack middleware as it execute much sooner in the request, or let people know that there is an up to 50% difference in results from using KernelEvents::REQUEST.

  • xjm committed 17a7f55 on 8.1.x 
    Issue #2474047 by arturogarrido, ejabrown, mikeburrelljr, acrosman,...

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.

  • xjm committed 17a7f55 on 8.3.x 
    Issue #2474047 by arturogarrido, ejabrown, mikeburrelljr, acrosman,...

  • xjm committed 17a7f55 on 8.3.x 
    Issue #2474047 by arturogarrido, ejabrown, mikeburrelljr, acrosman,...

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue tags: -Novice

Removing novice tag as it was for the initial patch, not the re-opened status.

  • xjm committed 17a7f55 on 8.4.x 
    Issue #2474047 by arturogarrido, ejabrown, mikeburrelljr, acrosman,...

  • xjm committed 17a7f55 on 8.4.x 
    Issue #2474047 by arturogarrido, ejabrown, mikeburrelljr, acrosman,...

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.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.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.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.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

quietone’s picture

quietone commented
Issue summary: View changes
Issue tags: +Needs change record updates, +DrupalSouth

Asked about this at DrupalSouth 2021, larowlan suggested adding the needs change record update tag. And then this issue can be closed when that is done.

See #28 for more details about what to add to the change record.

fubarhouse’s picture

fubarhouse commented

I've made some adjustments to the change record under the Notes header - hopefully this will make sense to somebody with more familiarity with the subject matter.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

This is a good start:

KernelEvents::REQUEST occurs significantly later in the request and could obscure observable numbers by up to 50%

However, I think we can be much more specific:

  1. What is the event later than? What is the thing we are comparing to?
  2. "Obscure" could mean shorter or longer. The way I read #28 the only direction the numbers can change is to be smaller
  3. What numbers? We're talking about time right? Let's at least use 'times', we probably can't specify a unit.
  4. I suspect the largest percentage differences will be on the shortest requests. We should either:
    1. Indicate the relationship between the percentage and the overall request length, or
    2. Quantify a range of values

    But considering the vast variability of systems I think 4.2 is in the too-hard (nigh impossible) basket so I'd go with 4.1

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Version: 11.x-dev » 8.0.x-dev
Status: Needs work » Fixed

This was committed

Status: Fixed » Closed (fixed)

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