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

Turn specs into PHP objects #3730

Closed
schlessera opened this issue Nov 14, 2019 · 2 comments
Closed

Turn specs into PHP objects #3730

schlessera opened this issue Nov 14, 2019 · 2 comments
Assignees
Labels
Groomed Punted Sanitizers WS:Perf Work stream for Metrics, Performance and Optimizer
Projects

Comments

@schlessera
Copy link
Collaborator

schlessera commented Nov 14, 2019

Feature description

We currently generate a big lump of array data from the AMP specs that we then load into PHP and process.

This is less than ideal for several reasons.

Arrays only know about strings

We have to manage an entire set of constants and always refer to these constants to access a specific index, so as to avoid simple typos introducing bugs.

With classes, this changes so that we can directly refer to properties, without the need for constants.
Arrays:

$attr_spec_rule[ AMP_Rule_Spec::VALUE_URL ][ AMP_Rule_Spec::ALLOW_EMPTY ]

Classes:

$attr_spec_rule->value_url->allow_empty

The IDE will help with autocompletion and show the available values, and typos are immediately highlighted.

Arrays cannot have logic

We have a lot of code that tests the existence of an element within the spec array first before reading and using it. All of this logic is tedious and bug-prone, and obfuscates the actual logic.

With classes, we can encapsulate a lot of this boilerplate logic into the classes, freeing the consuming code up to concentrate on its core logic instead.

Arrays:

$is_mandatory =
	isset( $attr_spec_rule[ AMP_Rule_Spec::MANDATORY ] )
		? (bool) $attr_spec_rule[ AMP_Rule_Spec::MANDATORY ]
		: false;

if ( $is_mandatory ) {
	$this->remove_node( $node );
	return false;
}

Classes:

if ( $attr_spec_rule->is_mandatory ) {
	$this->remove_node( $node );
	return false;
}

Arrays use a multiple of the memory of classes

For a structure like we have with these specs, arrays will waste a surprising lot of memory.

This is due to the fact that the indexes cannot be optimized for arrays, they are strings, and every single index for every single element will need to be stored separately as a string.

For classes, these are turned into properties, and the name of a property is only stored a single time in the declaration of the class. After that, the usage is highly optimized by PHP.

A quick and simple test shows that for a relatively simple structure, a change from arrays to classes yields memory savings of 62%: https://3v4l.org/rOJr5

This number will of course vary based on what the structure actually is, but it will be significant either way.

This directly relates to #2769

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@schlessera
Copy link
Collaborator Author

For turning the source of truth into PHP, we could stick with the Python parser and add a second PHP step, or we could omit the entire Python dependency and rebuild it in PHP from scratch.

The generated objects should contain the actual data in their declarations, so that we can offload everything into the opcache and autoloader.

This would mean that the parsing from PHP to bytecode would be skip at runtime because of the opcache. Additionally, starting from PHP 7.4, we could set this up so that the opcache portion can be persisted across requests, making the entire spec loading an instant operation without any performance impact.

@amedina amedina added this to Backlog in Ongoing Apr 1, 2020
@amedina amedina added the Enhancement New feature or improvement of an existing one label Apr 1, 2020
@amedina amedina removed the Enhancement New feature or improvement of an existing one label May 14, 2020
@kmyram kmyram added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 5, 2020
@kmyram kmyram added the Groomed label Nov 24, 2020
@westonruter westonruter moved this from Backlog to In Progress in Ongoing Dec 16, 2020
@kmyram kmyram added this to the v2.1 milestone Feb 2, 2021
@westonruter westonruter modified the milestones: v2.1, v2.2 Feb 11, 2021
@westonruter
Copy link
Member

Implemented in ampproject/amp-toolbox-php#100

@westonruter westonruter removed this from the v2.2 milestone Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groomed Punted Sanitizers WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
Ongoing
  
In Progress
Development

No branches or pull requests

4 participants