Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Application config compilation #26

Open
wants to merge 1 commit into
base: 2.4-develop
Choose a base branch
from

Conversation

antonkril
Copy link
Contributor

@antonkril antonkril commented May 8, 2023

Problem Statement

Magento's modular nature necessitates a complex configuration and metadata retrieval process. However, the inherent complexity of this process is exacerbated by the inconsistent implementation. Magento employs two opposing methods for retrieving developer configuration and metadata: the Just-In-Time approach and the Ahead-Of-Time approach.

The Just-In-Time Approach

In this approach, when the Magento application needs to obtain its configuration (di.xml, events.xml, system.xml, etc.), it reads the relevant configuration files from all modules, merges them, validates the merged configuration, and caches it.

The Ahead-Of-Time Approach

When operating in production mode, Magento assumes that all Dependency Injection (DI) and Interception metadata, as well as generated classes, have already been produced by the Magento compiler, a separate application.

These contradictory approaches lead to confusion and hinder the developer experience by:

  • Requiring developers to understand various methods of cache/metadata/generated code cleanup
  • Causing discrepancies in application behavior between developer and production modes
  • Lacking an easy and clear way to view the merged application configuration, necessitating the use of step debuggers, reading cache storage, or other tools
  • Deviating from how non-Magento developers typically work with their frameworks and applications

Proposed Solution

  • Remove the just-in-time approach from the Magento application
    • Introduce an alternative configuration component that will directly read the merged configuration from the filesystem (or another storage medium)
    • Eliminate the concept of developer configuration cache from the Magento application
  • Rely solely on the ahead-of-time approach
    • Develop a new generation of the compiler
    • Transfer the configuration reading, merging, and validation logic to the new compiler
    • Implement incremental configuration merging capability
    • Incorporate incremental compilation capability (later stage)
    • Add a filesystem watcher to initiate the incremental configuration merging and compilation functions

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@antonkril antonkril requested a review from a team as a code owner May 8, 2023 03:39
@rhoerr
Copy link
Contributor

rhoerr commented May 9, 2023

Thanks @antonkril! I really like that this would consolidate and simplify the configuration vs compilation aspects of the architecture, when there doesn't seem to be a strong reason for them to be treated differently (other than history). It would also move some load from Redis to Opcache, which is even better.

Point of clarity: When you say 'configuration', you're referring to various merged XML files, correct? -- {module}/etc/*.xml, UI components, etc -- and not to system configuration (core_config_data).

System configuration is an interesting and maybe problematic case where you have both XML file aspects (config.xml defaults) but also core_config_data, app/etc/config.php, and app/etc/env.php. What do you envision for that, if anything? Merge the config.xml defaults but otherwise leave it unchanged, with the 'Config' cache becoming a cache of actual configuration only?

Do you see using the same approach for layout cache, or leaving that unchanged? Layout cache having the wrinkle of database layout updates (like widgets).

@antonkril
Copy link
Contributor Author

Good questions, @rhoerr. IMO, layout modifications should be considered part of the "development" stage and should therefore reside in the "developer" tools. The same concept applies to Admin Configuration. Both could potentially evolve towards the push approach. However, it is a more controversial opinion, so I'd prefer to set it aside for the time being.

@damienwebdev
Copy link
Member

damienwebdev commented May 11, 2023

For disambiguation, can we call these two processes:

"The Pull Approach" - "Just-in-time"
"The Push Approach" - "Ahead-of-time"

The Angular community has the same features in their compiler as well, and the two terms are common there (as well as in the broader computer science/compiler community).

This difference can (and historically has) caused issues for Angular developers just like Magento developers in terms of differences and debugging capabilities. The Angular team elected to switch to AOT-alone in Angular 9 (Angular 2-8 used JIT in developer mode).

I think this is a good idea for certain. Anything that we can do to remove code from the runtime is a win.

@antonkril
Copy link
Contributor Author

antonkril commented May 11, 2023

Good point, @damienwebdev. I don't like the "just-in-time" term here. But consistency beats my preferences :). Updated the proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants