Performance can be increased significantly by firing multiple concurrent requests. With my provided patch I could decrease execution time locally by ~2/3.

CommentFileSizeAuthor
#15 3152000-15.patch7.02 KBarthur_lorenz
#13 3152000-13.patch7.46 KBarthur_lorenz
#12 3152000-12.patch8.13 KBarthur_lorenz
#9 3152000-9.patch6.69 KBarthur_lorenz
#7 3152000-7.patch6.7 KBarthur_lorenz
warmer_async_requests.patch6.34 KBarthur_lorenz
#4 3152000-4.patch7.63 KBarthur_lorenz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

arthur_lorenz created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, warmer_async_requests.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

arthur_lorenz’s picture

arthur_lorenz CreditAttribution: arthur_lorenz at Aperto GmbH - An IBM Company commented
arthur_lorenz’s picture

arthur_lorenz CreditAttribution: arthur_lorenz at Aperto GmbH - An IBM Company commented
FileSize
3152000-4.patch7.63 KB

Fixed some CS issues, added default config and imroved naming. Unfortunately I can't get the tests to run properly on my local environment right now, so let's hope tests are green :D

arthur_lorenz’s picture

arthur_lorenz CreditAttribution: arthur_lorenz at Aperto GmbH - An IBM Company commented
Status: Needs work » Needs review
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and commented
Status: Needs review » Needs work

This is awesome. I have been meaning to address this for a while. I wonder if we can remove the TODO item in favor of creating a ticket, or fixing it here. I have second thoughts about making the assync feature configurable, maybe we should make it always async if that is available to all Drupal installs.

Back to needs work to get clarification about the questions above.

arthur_lorenz’s picture

arthur_lorenz CreditAttribution: arthur_lorenz at 1xINTERNET commented
Status: Needs work » Needs review
FileSize
3152000-7.patch6.7 KB

Alright, I made the async requests mandatory, removed the TODO (feel free to create a ticket for it :)) and added a message after successful cache warming.

Status: Needs review » Needs work

The last submitted patch, 7: 3152000-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

arthur_lorenz’s picture

arthur_lorenz CreditAttribution: arthur_lorenz at 1xINTERNET commented
Status: Needs work » Needs review
FileSize
3152000-9.patch6.69 KB

Oh, I used a function that's only available for php >= 7.3. Fixed that

Status: Needs review » Needs work

The last submitted patch, 9: 3152000-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and commented

Any guesses on why tests are failing?

arthur_lorenz’s picture

arthur_lorenz CreditAttribution: arthur_lorenz at 1xINTERNET commented
Status: Needs work » Needs review
FileSize
3152000-12.patch8.13 KB

Yes, that was caused by a missing onReject callback. Fixed this and also fixed some CS issues.

arthur_lorenz’s picture

arthur_lorenz CreditAttribution: arthur_lorenz at 1xINTERNET commented
FileSize
3152000-13.patch7.46 KB

Whoopsie, deleted composer.json...

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and commented

I really like this change. My only question is weather or not we want to drop the Make asyc requests checkbox, and only allow configuring the number of parallel requests.

I will let my co-maintainer decide on that sense, unless there is a technical point I am missing that you Arthur want to clarify in that regard.

Thanks again for this awesome patch!!

arthur_lorenz’s picture

arthur_lorenz CreditAttribution: arthur_lorenz at 1xINTERNET commented
FileSize
3152000-15.patch7.02 KB

No there is no technical reason for not making async requests mandatory. I just wasn't sure how positive your reception to this change will be, so I decided to go with a smooth introduction of this feature :D

Updated the patch to get rid of that option completely.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and commented

This looks good. There is a debug log statement left by accident that I may remove on commit.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and commented
Status: Needs review » Fixed

This was merged to the new release with minor changes.

Status: Fixed » Closed (fixed)

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