-
Notifications
You must be signed in to change notification settings - Fork 1
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
PoC modules #10
base: master
Are you sure you want to change the base?
PoC modules #10
Conversation
Very cool, thanks for all the work @arnaud-lb. Some comments:
This sounds to me fine, but maybe it'd be good if modules could have an init.php or smth that lets the module run whatever impure stuff it needs to run. That file would then be scheduled to be included after modules are loaded?
That sounds like it'll slow down adoption as it essentially makes it a big game of chicken and egg where every dependency has to be modularized first.. But it's ok I guess 🙂
Sounds like a difficult restriction to keep. It might be fine in isolation, but combined with the above restriction you end up with the issue that if something has circular dependencies, it is not modularizable, and thus anything using it is also not modularizable, which sucks. If any restriction needs to be lifted I'd say it's this one.
this also sounds very restrictive to me that you cannot have a parameter type for a class that does not exist without causing issues.. Why is that so? I imagine you could do it like for local symbols, keep the param type hint as a mangled version if the dependency does not exist, and if anything is passed in then it'd fail.. but if it isn't used all good? Same for conditional
That sounds very promising already, obviously the dev downside is unfortunate but sounds like we can live with it, and maybe it could be improved by having some dev mode in php that does not include/validate all files at once in a module but rather just sets autoloaders per module and lets things be loaded as needed? That might be too risky I guess, as it probably wouldn't be able to validate things in a way that guarantee the module would then still work in prod mode
Smart idea I think, for tests we wouldn't even need the optimizations to be applied across the entire set of module chunks, each chunk being loaded/optimized as its own piece would be enough. |
This seems totally doable
Ok, thank you for the feedback. I will see if we can lift this one.
One reason is that static analysis would need the definition of all symbols, including parameter types. An other reason is that in order to make module loading fast, we have to link classes before caching them, which may require the know the definition of parameter and return types as well. Although this is not the case of existing code with optional dependency as parameter type, otherwise such code would not work. So if we abandon static analysis, we can probably lift this restriction in cases that work today. If we implemented multi-version support, being able to resolve all symbols at compile time would also make things simpler.
This could work indeed. I will think about this. One possible drawback is what happens if the symbol is declared later.
Yes, this would increase the risk of having divergent behaviors between dev and prod. |
Regarding the dev mode performance, would it make sense in terms of separation of concerns to have an additional mode Then userland scripts could provide multiple strategies for invalidating the module:
|
Agreed. I suggested something very similar in the "Performance regression" section :) Maybe this could be controlled by a require_modules() parameter, so the composer autoloader could enable module.ini-only validation depending on the module being loaded. E.g. enable that for vendor packages only, but not vendor packages installed from source. It's possible to watch files efficiently with inotify and similar APIs, and I think it's possible without a separate worker. However these APIs limit the number of watched items, so it may not work out of the box depending on os and project size. |
Very cool, Arnaud! I love it. Though of course I do have comments. :-) The big one is that I don't think referring to As you note, it's also a problem for the CLI. Where would the cache even go for a CLI command, though? I suppose to some extent that could be mitigated by smart module design; eg, many packages may want to be split into multiple modules to minimize the amount of excess/unused code that gets loaded. But that could reduce the effectiveness of any compiler optimizations, too. Where the "right" place is to chop up a package into modules is going to be an open question. Though, the reason I like Jordi's suggestion of init files per module. Rather than a hard-coded filename, though, could it just be another entry in the I really like the module chunks idea, which could also potentially help with minimizing the excess loading. Note that for some systems this would require changing their test namespace; Various conventions include putting It would probably be prudent for FIG to have a PSR on common chunk names and usage patterns. The way local visibility is described, would it work for properties and methods as well, or only classes/functions? We presumably want both. (Though as long as we're confident it could be done, visibility is a logical break point to split off to another RFC if we wanted to.) If I read correctly, there's something about Enums that makes them... incompatible with modules? Hackliy supported? I don't quite follow the limitation there or what we can do about it. My original writeup never came up with a name for "a unit of compilation." It's still just "X". :-) Any suggestions on what that should be called, if it's even relevant? I'm not clear if this part of the brainstorm is viable:
I don't know if this is fully handled by the chunks concept. Maybe it is. Thoughts? All in all, the top line performance numbers (~10% performance boost already, and the potential for another ~10%) make this extremely interesting and valuable to pursue. The primary concern is ensuring we do not hurt any valid common workflows along the way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I read "Local symbols are mangled in the symbol table, so that looking up a local class yields nothing. As a result, normal class lookup is unchanged, and no overhead is added." — how are you proposing the mangled form looks like?
Zend/zend.h
Outdated
@@ -225,6 +225,7 @@ struct _zend_class_entry { | |||
|
|||
union { | |||
struct { | |||
zend_string *module; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would having the name module
here, and also as internal.module
where it does something totally different, not be confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree. I've since switched to the name user_module
in most places, to avoid confusion with modules
(which refer to extensions), but forgot to rename this one.
Please keep in mind that the code here is very PoC-quality, and definitely needs some cleanup.
@@ -1010,3 +1017,24 @@ ZEND_FUNCTION(opcache_is_script_cached) | |||
|
|||
RETURN_BOOL(filename_is_in_cache(script_name)); | |||
} | |||
|
|||
ZEND_FUNCTION(require_modules) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that modules only work with opcache installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. As mentioned in the PR description, this is currently implemented in opcache for simplicity, but a proper implementation would move that somewhere else.
However, modules without are cache a slow to load (as seen in the "Wall time (cold)" benchmark results), and I would not recommend to use them without opcache.
@Crell thank you for the feedback!
I see. Unfortunately, right now I don't see a way to make modules as fast as baseline with full timestamp validation enabled. I will think about it. Such systems could support
For CLI, I was thinking of relying on the opcache file_cache to reduce compilation overhead. However, there are two issues with this: Currently, the file_cache is not considered stable, and it's not enabled by default (neither is opcache). If we can stabilize the file_cache, enabling opcache.file_cache by default (with opcache.file_cache_only) in CLI would be a nice improvement event without modules. Testing CLI performance with the file_cache is the next item in my TODO.
Agreed. Ideally, applications should split by feature/use-case. However, complex frameworks may quickly pull a lot of modules anyway.
The current PoC is designed to allow both cross-file and cross-module optimizations (although this is not implemented yet), even when loading modules one by one. This is possible because if the cache for a dependency is invalidated, any modules that depend on it are also invalidated, so any cross- optimizations are safe. Currently,
Agreed. Maybe
In theory, this can support chunks with the same namespace. The only limitation is that chunks must be in the same directory as the module.ini file. The following layout would be supported:
With the following configuration:
However it may be challenging for autoloaders to know when to load the
I didn't though about properties and methods yet. I have to think about it, but it could probably work similarly, yes. Currently, visibility of class members is already implemented with name mangling, without knowing ahead of time if we are fetching a private or protected property.
Yes, there is some difficulty with Enums and classes that extend internal classes, because these can not be linked and cached in their linked state, currently (on Windows for internal classes, and all platforms for enums). Currently the PoC works around this in a very hacky way, and this has to be resolved. This doesn't seem impossible and this would benefit performance on Windows (maybe).
At this point it's mostly a technical detail, and the real "unit of compilation" currently is individual files. However, the "unit of cache" will be the whole module tree (a module and its dependencies), although dependencies may be shared by multiple units. I think we can remove this concept from the writeup for now, unless we need it to explain something.
There are two parts in this:
Currently the first part is prevented entirely by disallowing I'm not sure about the second part, however the three use-cases I have in mind would be handled:
Agreed |
Anything like |
Hm. Does that mean the performance characteristics of these two samples would be the same? require_modules(['symfony/cache/module.ini', 'symfony/cache/valkey/module.ini']);
// vs
require_modules(['symfony/cache/module.ini']);
require_modules(['symfony/cache/valkey/module.ini']); If loading both modules in one command doesn't actually have an optimizer benefit, it's probably best to make it a single value rather than an array, for simplicity. (I expected that doing it together would give the optimizer more to work with, which is why I did it that way.) |
This would mean chunks are the only way to partially-load a module. I'm not sure about this, as it effectively means as soon as you are using modules, all current autoloaders break down. You have to go all-in on module-based loading, which is by design less fine-grained. Is disallowing requiring of a file in a module necessary, or just the easiest way to do it for the PoC? |
Yes
Yes this is what I understood, and I was not sure until now. I agree.
It was just the easiest way to do it for the PoC. It may be possible to allow loading of individual files, internally it would be very similar to how chunks would work. This brings a few questions:
|
f3a7a8f
to
b0daa46
Compare
This pull request is a proof-of-concept inspired by the Modules RFC Spec Brainstorm, with the additional goal of providing the compiler with a broader view of the codebase (encompassing the entire module tree).
The primary objective of this PoC is to evaluate whether modules can be implemented with reasonable performance despite this goal.
TL;DR:
Additional constraints
The following constraints were added (some are in the spec, but not all) :
Note: Modules being pure apart from class and function declarations is not enforced (yet) for uncached modules. We could enforce it by restricting which expressions can be used in top level statements, which functions can be called, delaying error handling, and delaying output (because of buffering handlers).
Circular dependencies between modules are not allowed (circular dependencies between files of a single module are allowed)Behavior
require_modules($descriptors)
, with$descriptors
an array ofmodule.ini
file paths (format detailed below)require_modules()
can be called in an autoloaderrequire_modules()
only has an effect on the current requestmodule.ini
are executedOther details:
module
statement, this is an errormodule
statement with a different name than the one declared inmodule.ini
, this is an errorFoo
, callingrequire_modules(["module.ini"])
is an error ifmodule.ini
declares moduleFoo
)module.ini format
The module.ini format is similar to what is described in https://github.com/Crell/php-rfcs/blob/master/modules/spec-brainstorm.md, but some compromises were made for ease of implementation, for now.
The file accepts the following entries:
module
: Defines the module's namespacefiles
: A space-separated list offnmatch()
patterns. Patterns are anchored to and relative to the directory containingmodule.ini
. Unlike inglob()
patterns,*
matches/
infnmatch()
patterns, sosrc/*.php
matches.php
files recursively insrc/
.exclude
: A space-separated list offnmatch()
patterns. Anchored to and relative to themodule.ini
file directory.Example:
Dependencies
Dependencies are all classes and functions used by a module. This includes implemented interfaces, parent classes, parameter types, return types, property types.
Classes referenced in
instanceof
, or in::class
, are not considered dependencies.Classes and functions defined in a file, but not declared during the first execution, are not inspected, and are allowed to have unmet dependencies. In the following example, if class
Dependency
does not exist, neither does classC
, so this file doesn't have unmet dependencies:Optional dependencies
Packages with optional dependencies may either use conditional declaration like the example above, to prevent errors during module loading, or move the dependent code in a separate module (potentially in a sub-namespace).
For example, the
symfony/cache
package may have a sub-module for each storage adapter.Conditional instantiation of optional dependencies is not really possible currently. E.g., this code will result in an error at compile time if
Dependency
does not exist:It may be possible to improve this.
Classes whom only some methods have an optional dependency (e.g. via a parameter type) must be split.
Execution order
During the first load, we compile all files to generate and install a class map autoloader. After that, we execute all files in alphabetical order, relying on the autoloader to satisfy any dependency.
Executing files in topological order would have been a possibility, but an autoloader is still required to resolve circular dependencies between classes during linking. For example, the following files have circular dependencies that can only be resolved by autoloading:
Instead of using a class map, it would be possible to just execute the remaining module files in the autoloader, since these are going to be executed anyway. This would result in a simpler implementation.
Internal classes
Currently, the implementation requires that all classes can be linked and cached in opcache. Unfortunately some classes can not be cached in their linked state (e.g. enums, and classes extending internal classes, at least on Windows). The PoC hacks around this, but a proper implementation would need to make these classes, or to relax this requirement (at the cost of performance).
Performance
Performance was measured on the Symfony Demo application after converting its vendor packages to modules. As the conversion required some tweaking ("fixing" circular dependencies, disabling some dependencies), I applied the same changes to the baseline.
Wall time (cached):
Modularized Symfony Demo is ~12% faster in production settings and ~3% slower in dev settings (due to the larger number of files to validate).
The same benchmark on an old laptop shows similar improvements in prod settings, but worth regression in dev settings:
Expand old laptop results
Wall time (cold):
In this benchmark, we measure the average time of a single web request on /en/blog, or a single CLI execution of bin/console. Both are considerably slower with modules: there are more files to compile, and the cache is cold. This is critical for the CLI as the cache is always cold.
Using a file cache (warmed up before the benchmark) mitigates this entirely for the web benchmark. The CLI is still slower, but orders of magnitude less, and mostly because the baseline is very fast.
We should consider loading the opcache extension by default in all SAPIs, and enabling the file cache by default in CLI.
Memory usage:
Number of declared classes:
SHM usage: +39%
Performance regressions
As seen above, modules are a few % slower in development settings (
opcache.validate_timestamps=1
), due to the larger amount of files to validate, and much slower during the first request (on a cold cache), due to the larger amount of files to compile.The cold cache slowdown may be an issue especially for CLI scripts, as the cache is always cold. This may be mitigated by enabling the file cache by default in CLI.
The timestamp validation overhead may old be mitigated in a few ways:
Local visibility
[Not implemented]
Not implemented yet, but this could be implemented with zero runtime cost, like this:
Modules chunks
[Not implemented]
Files with a
module
statement can not be required/included, so it is not possible to "sneak" a class in a foreign module. But also, modules can not have lazy-loaded / optionally-loaded files currently.It may be desirable lazy-loaded / optionally-loaded files may be desirable to, at least for tests: Test files should not be loaded most of the times, except when running tests, but tests must have access to local-visible symbols.
One way to allow chunks of modules to be loaded later, and optionally, would be to declare "module chunks" in the ini descriptor:
In the example above, the module
Foo\Bar
doesn't load tests by default, but tests can be loaded by callingrequire_module('module.ini', 'tests');
. Tests are in the same namespace as the rest of the module, and have access to package-visible symbols.TODO