Problem/Motivation

The method mailsystem_get_classes is executed in hook_init. As it is executed in hook_init and it is expected to be mostly static, it is safe to move it to cache_bootstrap. Moving it there will allow further improvements of cache speeds through this back-port of D8 feature.

Some still open questions:
1. Is this method really needed on init?
2. Can we make it lazy on the first mail call?

Proposed resolution

Just move the bin used for the cache to the cache_bootstrap one.

Remaining tasks

Discussion, patch and the rest.

User interface changes

None.

API changes

None. Implementation detail - move the cache layer.

Data model changes

None.

CommentFileSizeAuthor
#2 mailsystem-move-to-cache_bootstrap-2708415-D7-2.patch1.47 KBndobromirov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ndobromirov created an issue. See original summary.

ndobromirov’s picture

ndobromirov CreditAttribution: ndobromirov commented
Title: mailsystem_get_classes is heavy. » Move mailsystem persistent cache in cache_bootstrap
Issue summary: View changes
Status: Active » Needs review
FileSize
mailsystem-move-to-cache_bootstrap-2708415-D7-2.patch1.47 KB

Updated title and description.
Here is a POC patch.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented
Status: Needs review » Closed (duplicate)

Thanks for the patch and the issue @ndobromirov!

I believe this was already addressed on #1905544: Cache mailsystem_get_classes() between requests - which is now part of the latest release. Feel free to reopen if you feel otherwise.

ndobromirov’s picture

ndobromirov CreditAttribution: ndobromirov commented

Well the issue is a bit different :). It's not about adding a cache layer, as there was one already, but moving it's storage to a more appropriate place - cache_bootstrap.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented
Version: 7.x-2.34 » 7.x-2.x-dev
Status: Closed (duplicate) » Needs review

Ah yes, you're right @ndobromirov - clearly went to fast here :)

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented

1. Is this method really needed on init?

You may want to see the discussion on #2558713: Mail system should not be using hook_init

ndobromirov’s picture

ndobromirov CreditAttribution: ndobromirov commented
Status: Needs review » Closed (outdated)

Because the following 2 - I am closing this.

  1. ChainedFastBackend was deemed a performance killer in D8, due to APCu fragmentation.
  2. The init issue that you've linked is the better approach to speed the module up - move the code out of it.
SocialNicheGuru’s picture

SocialNicheGuru CreditAttribution: SocialNicheGuru commented

@ndobromirov can you site a source for the performance issue in D8?

ndobromirov’s picture

ndobromirov CreditAttribution: ndobromirov commented

Two examples showing proper internal issues in the ChainedFastBackend.
#2611400: ChainedFastBackend should not invalidate the whole fastBackend when doing a Set()
#2508417: ChainedFastBackend should have a centralized invalidation service