Problem/Motivation
The Dependency Injection Container allows us to dynamically register services and lazy-load them the first time we need them. This is great, but interacting directly with the ContainerBuilder can be slow.
Proposed resolution
Introduce use of Compiler Passes, which allow compiling bundle dependency injection container configurations to disk. It reads through the configurations, and then generates a PHP file, which comes packed with all the registered services. This will not only allow us to speed up the instantiation and use of the Container, but also start to use Symfony Bundles correctly.
Remaining tasks
- Review the patch and get it RTBC
- #1673162: Analyze performance of uncompiled and compiled DI containers
- #1708692: Bundle services aren't available in the request that enables the module
- #1784312: Stop doing so much pre-kernel bootstrapping
- #1759582: Figure out when to delete compiled DIC files
- #1716048: Do not boot bundle classes on every request
User interface changes
None.
API changes
Bundles can now use CompilerPasses to send service configuration to the dependency injection container. If you look at the current CoreBundle.php, we create a chain matcher object and add matcher objects to it. After the patch we can register a chain matcher service and simply tag the matcher services allowing both for easy extension within module provided Bundles. Also, it's much cleaner not to instantiate objects when building a DIC. And a tiny bit faster too.
Original report by Crell
This is a follow up to:
#1599108: Allow modules to register services and subscriber services (events)
#1673162: Analyze performance of uncompiled and compiled DI containers
#1675260: Implement PHP reading/writing secured against "leaky" script
Once we have a mechanism to compile the DIC to disk, we should start doing so. There are clear performance benefits, and those benefits will only grow larger the more we use the DIC.
Comment | File | Size | Author |
---|---|---|---|
#197 | 1706064-197.patch | 33.9 KB | no_commit_credit |
#197 | interdiff.txt | 1.97 KB | no_commit_credit |
#189 | interdiff-124-188.txt | 22.55 KB | effulgentsia |
#188 | dic-1706064-188.patch | 33.92 KB | effulgentsia |
#188 | interdiff.txt | 2.05 KB | effulgentsia |
Comments
Comment #1
katbailey CreditAttribution: katbailey commentedJust putting this here for when #1675260: Implement PHP reading/writing secured against "leaky" script lands ;-)
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedOops. I thought I was following this issue, but I guess I wasn't, so I missed #1, and added an alternate implementation in #1675260-69: Implement PHP reading/writing secured against "leaky" script. About half of it is the same, but at least a few hunks from #1 do some fancier Symfony kernel integration, so not closing this as a duplicate yet.
Comment #3
pwolanin CreditAttribution: pwolanin commentedDiscussed with effulgentsia this morning - this issue should not be committed unless the patch provides for a way to compile in development, deploy to production, and disable any writing of PHP in production.
Comment #4
catchComment #5
katbailey CreditAttribution: katbailey commentedOK, rerolled with the actual PhpStorage component that got in. I've also ripped out the stuff about metadata and file resources that I had borrowed from the Symfony implementation in the previous version of the patch, as well as the cache warmer stuff, because we need to figure out the whole compiled container invalidation issue and then we'll know what makes sense to borrow from Symfony.
So this is a very basic implementation and we'll still need to:
- figure out the logic around compiled container freshness (e.g. when modules are enabled/disabled that provide bundle classes)
- write tests
Comment #7
chx CreditAttribution: chx commentedWhat about this?
Comment #9
chx CreditAttribution: chx commentedSome cleanup and a very important fix: each test method needs its own container class or the world ends (my Apache segfaults for eg.). So the important change is:
Comment #11
pounardWhen you switch from the protected implementation to FileStorage one, it crashes: the protected implementation will write a SomeClass.php/ folder, with the protected file inside, and the FileStorage::exists() method will return TRUE on this folder detection if you switch meanwhile, and crash because it won't be able to either load it or delete it. This might be the source of testbots confusion if the php folder is not erased between various tests.
Comment #12
chx CreditAttribution: chx commentedYeah, we need to patch pifr to chmod recursively much like core does. That has nada to do with the fails in #10 thought, that is about drupalGetTestFiles scanning public:// and file_scan_directory freakin' out at the dirs it cant access. Trivial to fix. I have also moved the "make the class unique" logic into getContainerClass where it belongs but that's no functional change. Should be ready to go.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedName of 2nd param is $reset. Descriptions of both params is confusing.
A comment explaining why we need to suppress errors here would be nice.
Can we avoid a potentially expensive $storage->exists() call here by just doing a load() and then checking class_exists()?
Also, it would be nice if this could be robust to a storage implementation that can't write: i.e., if after load(), class_exists() returns FALSE, then set $this->container to $container.
Also, I think module_implements_reset() (and maybe drupal_flush_all_caches()) needs to do a $storage->delete().
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedEven better than this kind of prefix (ahem, suffix), would be if we could get a hash of the enabled modules, since the container should always be the same for a given set of enabled modules. If that's expensive, perhaps we can cache that hash?
If the class name is unique to the list of enabled modules, then a class_exists() prior to load() should be sufficient, I think. If the class is already in memory for a subsequent test run, we should be able to just use it.
Comment #15
pounardI also noticed that on my box, the protected files chmod set strange mod bits, and my os displays "? ??? ? ? ??" on
ls -l
which is not a good sign.Comment #16
pounardI agree.
I'm not sure if this is linked, but I got bugs due to failed file write and I was going WSOD because of not existing container. class_exists() calls are mandatory here to provide a fallback behavior.
Comment #17
chx CreditAttribution: chx commentedThanks for the very insightful review, effulgentsia! The results are a lot, lot cleaner I hope.
Comment #18
chx CreditAttribution: chx commented#15, the chmod we use are bog standard, if it does not work for you, that's a separate issue, please file a proper bug report.
Comment #19
pounardI didn't try to reproduce the failure I experienced yet with #17 but I think it's better reading the code, it provides an ultimate fallback in case the container class does not exists which should fix it. This fallback case is not something that should be silent IMO, I don't know if we have a logger yet this early during bootstrap, but it might worth it to log it somewhere that the container did a rebuild, and that is not something that should happen in normal/production runtime.
Comment #20
pounardI do not agree for the
module_implements_reset()
, but regardingdrupal_flush_all_caches()
, it is sometime revelant, sometime not. I don't think you want to mess up with generated PHP files on a production server, even when you clear the caches (which can arbitrarily triggered by some modules, while compiling really make sense to be triggered when you enable or disable modules, but not really when you flush the caches).Comment #21
chx CreditAttribution: chx commentedI do think that the cleanup is a followup too :) #1759582: Figure out when to delete compiled DIC files the amount of garbage left behind is really small, these are like 10K files, even a couple hundred of them takes no significant disk space.
Comment #22
pounardIndeed. As I said #17 looks better, needs some love and testing, I'm quite sure there are use cases we didn't think about, such as, for a example, a "production" or "development" boolean in configuration to disable compilation (for devel environments).
Comment #23
aspilicious CreditAttribution: aspilicious commentedIf you look at the changes it's rly hard to swap between compiling and not compiling. The entire logic is different. Even the bundles need to be build differently. So I don't see a possibility for such a switch. Why not a drush command that refreshes the compiled container? Or a UI button in the performance settings.
Comment #24
pounardAll you have to do is skip the load and fallback in building the compiler at runtime the exact same way it's being done (and not dumping it).
Comment #25
aspilicious CreditAttribution: aspilicious commentedThats *not* true
1) Adding subscribers differs if you compile or not
2) Compilerpass systems don't work if you don't compile
3) ....
It's more than just "to compile or not to compile"
Symfony wants you to compile the container they build a lot of their logic on top of that principle.
Why would we else have those ugly workarounds in code?
An example from the patch
Those two things do the same thing but one is based on compiling the other is not.
EDIT:
Maybe I am confused by your comment. Do you propose to compile the container and just not dump it? In that case it is possible but I'm not sure that is a good idea either (but I don't have any technical reasons for that)
Comment #26
pounardOk, I did not use the right word, by compiling I meant "compile AND dump it". I meant no dumping to file of course.
EDIT: Real definition of a compiler is a program that transform any language code to any other language code. Symfony abusively uses the term compiler, because if you disable the dumping phase, you are not generating code in another language, but just manipulating objects into memory. Plus Symfony Compiler actually do not parse any file (in our use case particularly) because all the symbols are generated using PHP (see Drupal\Core\DependencyInjection\Compiler\RegisterKernelListenersPass::process() for example). So you are just doing a subset of compilation. That's what brought my wrong wording that brought confusion (wrong according to Symfony terminology). See http://en.wikipedia.org/wiki/Compiler
Comment #27
pounardSome Symfony developers actually do not dump their compiler at all during active development. Let me give you a user story: As a developer, I don't care about clicking a button to clear my cache, I just want no cache at all.
Comment #28
chx CreditAttribution: chx commentedThen use a storage backend which is not writeable. One is even included in core. It will rebuild on every page load. It'll be slower but it'll do just that.
Comment #29
chx CreditAttribution: chx commentedWhat Alex meant is this: because the classname is not dependent on the list of modules we need to either delete the file (so it gets rebuilt) on module changes or we need to make it the name of a hash of enabled modules. I did the latter, resolving a @TODO in DrupalKernel while doing so.
Comment #31
chx CreditAttribution: chx commentedRerolled against HEAD.
Comment #33
chx CreditAttribution: chx commentedThe only change is that I needed to add a character in front of the hash because it might start with a number and that's not a valid PHP identifier.
Comment #35
aspilicious CreditAttribution: aspilicious commented#33: 1706064_31.patch queued for re-testing.
Comment #37
pounardI have one question though, why hashing the compiler name using the module list?
Comment #38
chx CreditAttribution: chx commentedBecause the DIC only depends on the bundles available which depends on the modules.
Comment #39
pounardSomething that hits me is that the module list is not supposed to change on a site. The only cases where you don't have the same module list could be in CLI scripts, or during install and update. In those cases, and because you use them only for maintainance tasks, I'd prefer not having a dump at all instead of dumping a new file. That's a matter of taste I guess, but I think ofuscating that much the file name sounds a bit too much IMO, because if the module list change, you won't reuse the old kernel ever, so you'd better delete it and create a new one with the same name.
Comment #40
chx CreditAttribution: chx commentedNot everyone uses CLI. For them the module list will absolutely change on module enable and disable. You might want to say that "hey let's just write a new DIC on module enable" but the timing of that is insanely tricky -- there is an already existing issue about the DIC and module_enable so I wanted to desperately avoid that here. You might call it obfuscating the flilename, but let's face it, that's hardly something you need to bother with -- you will never edit it or anything. And, if as you say, it's not changing anyways, you will know that the hash-named file in the backtrace is the DIC, anyways.
Comment #41
pounardThen comment it in the patch please, it seems arbitrary when just reading the code as-is.
Comment #42
chx CreditAttribution: chx commentedAdded.
Comment #43
pounardAnyway your explaination is fair enough, I agree to keep the hash.
Comment #44
aspilicious CreditAttribution: aspilicious commentedPossibly needs a doc review. I found these tiny issues. Is there anything that has to be done before we can get this in?
Comment #45
podarok+ // While the default Synfomy class name only depends on the environment,
+ // While the default Symfony class name only depends on the environment,
Comment #46
podarok;)
Comment #47
cosmicdreams CreditAttribution: cosmicdreams commentedThis fixes #45
Comment #48
cosmicdreams CreditAttribution: cosmicdreams commentedComment #49
chx CreditAttribution: chx commentedBut not #44 it seems , at least the kernel two dots is still in there.
Comment #50
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedSymfomy -> Symfony
Comment #51
podarokfix #50
Comment #52
podarokall doc fixes from #44
Comment #54
podarok#52: 1706064_compile_the_injection_container__52.patch queued for re-testing.
Comment #55
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #57
aspilicious CreditAttribution: aspilicious commented#55: 1706064-compile-the-injection-container-55.patch queued for re-testing.
Comment #58
aspilicious CreditAttribution: aspilicious commentedI asked katbailey her opinion and she had 2 valid questions:
1) Do we ant to compile while testing?
2) Do we test the DIC compiling process itself somewhere?
Comment #59
pounardThe whole idea in testing is having a mock container with only the services needed for the test (especially in unit testing), we shouldn't compile that, and use a basic container (no compiler pass, etc...).
For functional tests, we have two containers, one is the testing site/script and the other is the tested site. The interesting one is the tested site's one, it should be compiled normally after the fake site install as it would be on a normal site so we ensure tests are the closest possible to what happens in real life.
Does that make sense?
Comment #60
chx CreditAttribution: chx commentedWe do not have a non-compiling version any more, or that seems to be the point? However, there are twice as many test files (257 : 493 ) NOT containing the string 'static $modules' than those containing it and so the question rises -- can we get a major speedup by pre-creating the DIC for profiles and copying it in place?
Comment #61
Crell CreditAttribution: Crell commentedchx: My gut reaction is yes, if we knew what DIC to compile and ship. Is there some way that the host system could detect the install profile that tests are using, compile a DIC for that once at the start of the test process, and then just continue reusing that? I don't know if that's possible but it would be sweet if so.
Comment #62
pounardMy feeling is that DIC will change and be different for each test. If that was possible, I'd say yes, but we would end up by mocking up a false site that diverge from a normal site and tests wouldn't be 100% reliable. Each test is different and builds a false site accordingly to what it tests exactly.
Plus there is the unit test problem, you need to be able to build DICs for unit tests, and those shouldn't be compiled since the unit test would be run only once and each unit test class would be different.
Comment #63
ygerasimov CreditAttribution: ygerasimov commentedAfter patch from #1702080: Introduce canonical FileStorage + (regular) cache landed we have conflict in bootstrap.inc file. I have tried to resolve it but during installation got error:
Please add the class to service "cache.config" even if it is constructed by a factory since we might need to add method calls based on compile-time checks.
I am not quite sure how to fix this conflict.
Regarding having compiled DIC for tests we can have option in test class definition what DIC to use and ship some standard ones. So we will have immediate benefit from using already existing DIC's. Surely this will require tests to have fixed set of modules.
Or are we talking about keeping DICs of each test somewhere in separate place and if next running test builds a list of required modules and sees that DIC with same set of modules exists -- reuse it?
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedComment #65
aspilicious CreditAttribution: aspilicious commented+ if ($container->has('request') && $container->has('content_negotiation') && $container->isScopeActive('request')) {
I think the following should be enough
+ if ($container->isScopeActive('request') && $container->has('content_negotiation'))
{Comment #66
chx CreditAttribution: chx commentedSigh. Most definitely won't be enough:
http://api.drupal.org/api/drupal/core!vendor!symfony!dependency-injectio...
Returns whether this scope is currently active
This does not actually check if the passed scope actually exists.
Comment #67
chx CreditAttribution: chx commentedOh, and here is how I found it -- I was unsure so I have thrown isScopeActive into Google and looked at the first result...
Am I very negative / hostile / etc again?
Comment #68
katbailey CreditAttribution: katbailey commentedYes, not to mention unfair.
The 'request' service is a synthetic service that is defined in the CoreBundle - so if we are dealing with the full DIC (not just the bootstrap DIC) the
$container->has('request')
check will always return true even when outside of request scope. And if$container->has('content_negotiation')
returns TRUE then the check for the request service will certainly return TRUE.At any rate, this certainly needs more reviews.
Comment #69
sunTestBase::prepareEnvironment() rebuilds a fresh container already (identical call) and this call in WebTestBase::setUp() is executed only shortly after. I wonder whether we can remove this second/duplicate container rebuild?
huh?
If this change to Symfony's code is intended, then we should add an inline comment what the hack is doing and whether and when it will be obsolete at some point?
Comment #70
effulgentsia CreditAttribution: effulgentsia commentedOops. No, that was just left over from me debugging.
Comment #71
chx CreditAttribution: chx commentedThen I apologize -- I will try to avoid in the future (I recognized myself, that's something). Edit: https://twitter.com/chx/status/243888508949901313
As for the code,
Why? There is a factory anyways.
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedRe #71, see #63. I think it has something to do with being able to reflect during compilation if method calls are added to the service definition.
Comment #73
pwolanin CreditAttribution: pwolanin commentedSo, I think it would be helpful to have a test or even test setup that set core to use FileReadOnlyStorage - so we can verify that nothing in core has a hard dependence on the success of writing out to disk.
Also, I think this needs work:
I think the dumpDrupalContainer() method needs to return the value of the save(), or return FALSE at the start if the storage is not writable. I don't think the code calling dumpDrupalContainer() should have to test the capability of the storage object.
I would also think we would want some kind of log warning if the call returns FALSE.
Comment #74
chx CreditAttribution: chx commentedWe can get rid of writeable, sure, it's not essential since the logic rearrange a couple dozen followups back :)
Comment #75
pwolanin CreditAttribution: pwolanin commented@chx - I think it's ok to have that method, I just think it's being called in the wrong place. It should be called in dumpDrupalContainer()
Comment #76
pwolanin CreditAttribution: pwolanin commented@chx - I think it's ok to have that method, I just think it's being called in the wrong place. It should be called in dumpDrupalContainer()
Comment #77
scor CreditAttribution: scor commentedlooks like a WSCCI issue
Comment #78
chx CreditAttribution: chx commentedIt's all yours.
Comment #79
katbailey CreditAttribution: katbailey commentedTaking a pass at this...
Comment #80
katbailey CreditAttribution: katbailey commentedOK, this makes the changes suggested by @pwolanin in #73. I've added a unit test for testing the behavior with and without compilation (i.e. in the latter case using the FileReadOnlyStorage implementation). In order to make the kernel unit testable I changed it so that we can optionally pass in a $system_list array, i.e. as opposed to the previous version of the patch which called system_list() and passed that to the kernel. This way, for unit testing purposes you can pass in a fixed list of modules, whereas under normal circumstances the kernel will be responsible for generating that list (which after #1784312: Stop doing so much pre-kernel bootstrapping and #1331486: Move module_invoke_*() and friends to an Extensions class get figured out will not require any crazy bootstrapping).
Comment #81
katbailey CreditAttribution: katbailey commentedActually, there's no need for the unit test to get the container from drupal_container() when it can just get it directly from the kernel. We still need to muck about with drupal_container() unfortunately because it gets called from within the kernel so we need to reset it afterwards.
Comment #82
RobLoachThis looks pretty good to me. I cleaned up a couple comments, but I'm thinking this is RTBC as long as we have follow ups set up for anything missing.
Comment #84
scor CreditAttribution: scor commented#81: 1706064-compile-the-injection-container-81.patch queued for re-testing.
Comment #85
aspilicious CreditAttribution: aspilicious commentedSymfony\Component\DependencyInjection\ContainerBuilder::merge() must be an instance of Symfony\Component\DependencyInjection\ContainerBuilder, instance of ce97b921f0da78163c923d45216c0a159f8ca74e54712683c6c372d584662dbbc
Comment #86
RobLoach#81: 1706064-compile-the-injection-container-81.patch queued for re-testing.
Comment #87
effulgentsia CreditAttribution: effulgentsia commentedComment #89
RobLoach#87: 1706064-87.patch queued for re-testing.
Comment #91
scor CreditAttribution: scor commentedlooks like the new
drupal_container(NULL, TRUE);
that @effulgentsia introduced in setUp() in the last patch solved some of the failures but introduced new ones :(. A lot of them are due to the config system not be initialized after that call to drupal_container(), e.g.config('search.settings')->get('active_modules')
or any other call to config()->get() returns NULL even after the kernel boot.Comment #92
Crell CreditAttribution: Crell commentedPerhaps we're going about this the wrong way. Rather than trying to dance around creating a compiled DIC and swapping it out at runtime, why don't we just allow for a gien run to not use a compiled DIC, or to recompile on the fly? Vis, Symfony has a concept of a "dev mode, where things get rebuilt every request, much as the "rebuild theme registry on every request" toggle works for Drupal. What if we were to allow the rebuild to happen in that mode, don't give a crap that it's going to be slow, and then redirect back to self without whatever flag we use to indicate "force into dev mode"?
Remember we want to also allow rebuilding the DIC from the command line, so we're going to need to allow out of band rebuilds anyway...
Comment #93
katbailey CreditAttribution: katbailey commentedNow that the router is in we need another compiler pass to deal with setting up the matcher services - here's an attempt at that. I also added the use of the debug flag to prevent compiling and am setting that to true during installation. For now I just want to see what testbot has to say...
Comment #94
katbailey CreditAttribution: katbailey commentedComment #95
pounardHappy test bot is happy.
Comment #96
podaroka small typo - all other looks good for me
Comment #97
cosmicdreams CreditAttribution: cosmicdreams commentedExtra whitespace
Comment #98
katbailey CreditAttribution: katbailey commentedWow, I really wasn't expecting such compliance from testbot.
Ok, so I fixed up some whitespace and beefed up the unit test for DIC compilation.
What I'd love to get people looking at in particular at this stage are
... as well as just general reviews of course - it's just that those are the areas I feel really need scrutiny right now.
This is a really important patch that has repercussions for how we solve other problems relating to the DIC (because without it we cannot use compiler passes).
Comment #100
tim.plunkettRerolled for #1803338: 403 and 404 pages call error_log cluttering server error logs
Comment #102
pounard@katbailey the module list hash sounds robust, as long as there is nothing that dynamically (following variables or such) registers services. But the whole goal of registration is to have something monolithic, so I think it's OK. As long as it is possible to delete it in order to force a rebuild, we're good to go.
I have a not about the
writable()
method (which probably should be namedisWritable()
IMHO, but that's another debate) is that thePhpStorage\FileStorage
class can be non writeable depending on the directory it's working in, so I think it should return something likeis_writable($this->directory)
or such (I'm guessing the variable name here). It will force astat()
call I guess, but it's important to reflect reality of the OS here, else we could end up with some weird behaviors in some atypic or misdone site installs.class RegisterMatchersPass
doesn't respect coding standard (missing blank lines at the top and the bottom of the class definition).Your compiler pass looks OK but I'm not confident enough with SF2 compiler, so I'd like more reviews to confirm that. I'm just reading the patch here not doing a full review by applying it, but I think you can omit the
PassConfig::TYPE_AFTER_REMOVING
? I'm not sure there but in the SF2 documentation example, which is a use case which looks a lot like ours (I might be wrong, tell me if so) they don't specify any behavior here and leave the default. If I understand it well, the goal is to register the matchers for the chain matcher? Then I guess you are doing it right! Tagging is exactly what we want here.I'm still not convinced by the procedural system_list() call into the kernel, I think the module list should be either injected either read from the early container. I don't like having two containers, even if they get merged by compile they still are both instanciated on every bootstrap, but as long as we have it, we could inject the module list as kernel parameters when instanciating it?
Comment #103
effulgentsia CreditAttribution: effulgentsia commentedIs the goal to eventually replace it with an injected Extensions service once #1331486: Move module_invoke_*() and friends to an Extensions class is resolved? In the meantime, we could possibly inject a ConfigFactory service, since as of #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config, system_list() is just a config get, but not sure if that makes sense if we'll eventually switch to an Extensions service.
Right. That's on the chopping block in #1784312: Stop doing so much pre-kernel bootstrapping.
Comment #104
katbailey CreditAttribution: katbailey commentedAfter updating from HEAD, web tests were breaking with DIC compilation because the cachedstorage was getting hardcoded as e.g.
So I've added a $rebuild parameter to the Kernel constructor which sets a 'stale' property on it and forces the cached container file to get deleted. Seems to work. It's either that or don't compile the container during test runs, I'm not sure which makes more sense... thoughts?
Comment #106
katbailey CreditAttribution: katbailey commented#104: 1706064.compiled-dic.104.patch queued for re-testing.
Comment #108
katbailey CreditAttribution: katbailey commentedOK, here's a version without the stale/rebuild business - it just uses debug mode when running the tests so that the container doesn't get compiled.
Re #103, yes this will all change with #1331486: Move module_invoke_*() and friends to an Extensions class - I think we should retain the flexibility of being able to pass in a fixed list of enabled modules, but it shouldn't be necessary to instantiate anything to inject into the kernel. With the ExtensionHandler patch, the kernel is responsible for instantiating it - and if it receives a fixed list of modules in its constructor it can set that on the ExtensionHandler it creates.
Comment #109
Crell CreditAttribution: Crell commentedAwesome work, Kat! This makes a ton of sense to me reading through it. I think there's only one major concern I have, plus various and sundry smaller ones. See below.
This is also going to change any minute now, I think: #1764474: Make Cache interface and backends use the DIC
I'm curious why you're registering these this way, with method, rather than ass addMethod() on the nested_matcher service. I'm not against it, but I want to understand why. (Which may indicate a need for comments.)
New coding standards say this shouldn't be use'd, and just referenced inline with \InvalidArgumentException.
I feel as though these should be separate compiler passes, since they're operating on 2 different tags. There's no performance impact to that, is there?
Although now I see why you're registering the matcher services the way you are. Nifty. We'll need to carefully document that, though, since it's non-obvious.
I understand from earlier comments why this is being done, and I'm OK with it as a temporary measure, but it needs a @todo.
Oh, purdy!
However, shouldn't the mode be in the hash, not just the module list? Even if we're not mode switching yet it seems a good idea to be prepared for it.
This is the big one: Might this result in lots of older files? It seems like we'd need some sort of garbage collection here sooner or later.
Why can't we just use a try-catch in this method like a normal person? :-)
Why not just stick $storage in an object property, since we're going to be using it on all requests anyway?
Why do we need to strip comments? It's not like the code is getting sent over the network so we need to worry about bytes. If it's a security issue, it should be commented as such. If not, let's leave comments in to help with debugging. (Requiring that people switch to debug mode means we get heisenbugs.)
camelCase, please.
Comment #110
katbailey CreditAttribution: katbailey commentedThis patch addresses most of #109, but it does not address "the big one" :-/ See #1706064-21: Compile the Injection Container to disk - please oh please can we leave this for a follow-up?
As to some of the other points...
It now adds 'Debug' to the class name if we're in debug mode, which is what the Symfony one does - is that ok?
There's no actual exception being thrown here - we just want to log the fact that the container isn't compilable if we're using the read-only storage mechanism.
That was copied from Symfony code - I've ripped it out pending someone coming up with a reason why we need it.
See the interdiff for how the rest of the points were addressed.
Another change I made here is to not use the $debug property as the basis on which to decide whether or not to compile. Instead I've added a more explicit $useCompiledContainer property and added a parameter for it to the constructor, defaulting to TRUE.
Comment #111
Crell CreditAttribution: Crell commentedI'm fine with punting on garbage collection as long as there's a @todo on it. Deal? :-)
For the container name, I figured we'd append the mode (prod or dev) for future extension if we ever add another mode, but I don't feel strongly about it.
Otherwise, I'm pretty OK with this. chx, anything else on your end?
Comment #112
aspilicious CreditAttribution: aspilicious commentedI reviewed this patch and asked for more opinion in irc (mostly from chx). And they didn't see any big problems.
If #111 is done and the folowing nitpicks than I possibly can rtbc.
Please shorten the firsts sentence so it fits on one line.
Comment these things
Comment #113
Fabianx CreditAttribution: Fabianx commentedWhile on performance testing, applied the patch and saw a nice speed up in XHProf:
100 nodes, node.tpl.php from: 2064 ms down to 1965 ms
So 100ms saved (with overhead of XHProf) :-).
100 ms saved also with Twig ... Not more / not less savings.
Comment #114
katbailey CreditAttribution: katbailey commentedAddresses #111 and #112.
Re #113 - nice! :-)
Comment #115
sunI don't quite understand why and how it is possible to register an interface?
Won't this hide other errors, too? And what if the inaccessible $dir is the only $dir being requested? Shouldn't there be some error handling?
I think this lengthy @todo can be removed (and should go into DrupalKernel's constructor phpDoc instead).
The system list holds more than enabled modules. It contains all enabled extensions, so the hash needs to be based on the entire $list, and DrupalKernel phpDoc/comments should be adjusted accordingly.
Let's type-hint $system_list as array, and also add phpDoc for all the parameters here.
1) The array_keys() is no longer necessary.
2) Since you're skipping the call to system_list(), I wonder who and what calls it -- we need to make sure that drupal_get_filename() is still primed with the file paths of extensions. Otherwise, that will trigger very nasty full filesystem scans.
3) This effectively adds yet another cache to system_list(), which means that its data is cached in:
- cache_bootstrap:bootstrap_modules
- cache_bootstrap:system_list
- cache_config:system.module
- cache_config:system.theme
- PhpStorage:service_container
The logic in initializeContainer() can be vastly simplified by reversing the if/else conditions.
I think $class should be either rooted by prefixing \ or the compiled class should be in a certain namespace that is explicitly used and referenced. Otherwise, the $class name is relative and the classloader will try to find the class name in all registered namespaces.
Comment #116
chx CreditAttribution: chx commented#1708692: Bundle services aren't available in the request that enables the module merged into this one.
Comment #117
chx CreditAttribution: chx commentedWrong interdiff.
Comment #118
chx CreditAttribution: chx commentedDisregard #116 / #117 I will just move to the other issue.
Comment #120
chx CreditAttribution: chx commented> I don't quite understand why and how it is possible to register an interface?
It doesn't register an interface, as it uses a factory class. AFAIK the interface is for @return documentation only.
> if (is_dir($dir) && $handle = @opendir($dir)) {
> Won't this hide other errors, too? And what if the inaccessible $dir is the only $dir being requested? Shouldn't there be some error handling?
Perhaps -- but is there a point? It was already not doing anything if opendir() failed. If errors arent displayed these warnings went into watchdog , quite possibly unseen. I added an else - watchdog.
> I think this lengthy @todo can be removed (and should go into DrupalKernel's constructor phpDoc instead).
Moved.
> The system list holds more than enabled modules. It contains all enabled extensions, so the hash needs to be based on the entire $list, and DrupalKernel phpDoc/comments should be adjusted accordingly.
Except we do not use those. We only use the bundles of modules. Edit: none the less I fixed this.
> The array_keys() is no longer necessary.
Removed. Edit: it'll be in next the patch.
> Since you're skipping the call to system_list()
But module_load_all calls module_list which calls system_list.
> The logic in initializeContainer() can be vastly simplified by reversing the if/else conditions.
Sure.
> I think $class should be either rooted by prefixing \
Done. I do not think the PhpDumper we use supports
namespace
.Comment #122
chx CreditAttribution: chx commentedAh, OK.
Comment #124
chx CreditAttribution: chx commentedComment #125
sunThe primary reason for all this compiling of the container is performance - however, testbot durations on the full core test suite are ranging from neutral impact to 4 minutes performance decrease (no solid data available).
Can we do some benchmarks on cold + warm environments here to make sure that the entire extra amount of code and operations is worth the buck?
Comment #126
pounardTests are supposed to rebuilt the site at almost each and every test being run (or at least for each test case class), so caching the compiler in that specific context doesn't serve any purpose (or almost none). I think this would explain why the testbot doesn't see its performances improved much.
Comment #127
Anonymous (not verified) CreditAttribution: Anonymous commentedsun: i agree with you, this issue makes me want to just bang my head on the desk.
but unfortunately we decided back in july that we're ok with drupal's performance sucking without php cache files, so i think we just need to get this in.
Comment #128
aspilicious CreditAttribution: aspilicious commentedI just want to note that we turn off the compiling for testing purposes, so the testbot wont notice any improvements. Correct me if I'm wrong and I noticed a 32 min testbot in other issues. Apparantly it depends on the bot.
Comment #129
xjmBumping priority since #1708692: Bundle services aren't available in the request that enables the module was postponed on this, and tagging since that issue blocks VDC.
Comment #130
chx CreditAttribution: chx commentedIf benchmarks is the only concern left then we are good to go: #1673162: Analyze performance of uncompiled and compiled DI containers. Benchmarking this right now, without having a lot of services is not resulting in a meaningful speedup. I benchmarked the frontpage with ab and it's neither slower nor faster.
Comment #131
Fabianx CreditAttribution: Fabianx commentedAgree with the RTBC, I tested with unpatched core and core with TWIG, and as said in #113 got a 100ms speedup (with XHProf enabled) and rendering 100 nodes on /node compared to non-compiled DIC.
Comment #132
webchickI think it makes sense for catch to look at this one.
Comment #133
RobLoachUpdated the issue summary. If you find any issues or feel it needs more, then please feel free to be bold.
Comment #133.0
RobLoachdfsa
Comment #133.1
RobLoachfdsa
Comment #133.2
chx CreditAttribution: chx commentedCopy of the revision from October 15, 2012 - 10:47.
Comment #134
sunStale comment?
As a Drupal developer who did not participate in this issue, it is not clear to me
- why this class is needed
- what it is doing
- what data it operates on
- from where it is called
- what the process() method code is doing, and why
- etc.
The fully qualified class name is not always used.
Comment #135
effulgentsia CreditAttribution: effulgentsia commentedI suspect the reason for performance degradation in testbot environment is that this patch runs compiler passes, and those are very slow, because they're by definition not designed for runtime. So, running them to setup each test is quite a hit. Perhaps in a follow up, we can explore a precompiled container for the 'testing' profile, so that at least all tests that don't enable any test-specific modules can be faster?
Unlike #130, I saw a 2% improvement with this patch for the front page on a warm environment (49ms vs. 50ms). That's *in spite of* doubling the number of services CoreBundle adds to the DIC with this patch (in HEAD, all the event subscribers are not services, because of the lack of compiler pass support). While 2% is perhaps not that exciting at this time, it will grow as more services get added/used.
For cold environment, I configured my settings.php to use FileReadOnlyStorage as the PHP storage implementation and commented out
$error = 'Container cannot be written to disk';
from DrupalKernel (benchmarks are useless in the presence of watchdog() statements). Here, I saw 20% performance degradation (60ms vs. 50ms) due to the slow compiler pass process. This is an edge case though: a site that wants to use read-only storage can add precompiled DICs.Comment #136
chx CreditAttribution: chx commentedUpdated comment in system_list
Added missing doxygen to classes and methods. Didn't replicate Symfony handbook pages so the why / from where / etc is not
> The fully qualified class name is not always used.
I must be blind. I thought that the fully_qualified_classname is used immediately, unconditionally right on the next line that is even quoted.If this is a coded complaint for "we are not smashing words together' that's a valid one, changed to class_name.
Comment #137
effulgentsia CreditAttribution: effulgentsia commentedI wonder if "The fully qualified class name is not always used." was meant to say "why are we calling class_exists() on a non-qualified class name"?
Can we also pass FALSE as the 2nd param to those class_exists() calls, since we don't want autoloading there?
Comment #138
chx CreditAttribution: chx commentedclass_exists does not take the namespace it is inside into account. It just doesn't. Run:
get false, true, false, true. Now, adding FALSE, sure, here it comes. And, apparently I forgot to re-run diff when I found class_name, so that's also present in this one only.
Comment #139
chx CreditAttribution: chx commentedTo sum up: PhpDumper does not support namespaces. Therefore these classes are always in the root. class_exists operates always as if it would be ran from root therefore it is not necessary to add a \ in front. The FQCN passed to new is not necessary either because the chances of the Drupal\Core namespace containing a class called ca4f15de4f70a3e1786208893a9ac779ebeb6d97550400a99162c84aac5718909Prod is quite low. But, I was asked so I added it. Now I wish I didn't. I didn't expect such a storm of followups :(
Comment #140
pounard@#135
Which version of PHP are you running, and fast is your machine? Working on my dev box (core i3 running on Gentoo, custom configured PHP 5.3.x, running PHP-FPM with APC, tuned MySQL connected via socket as database) a normal front page run right after install with D8 is between 100 and 150ms (3x your figures). As a comparison, it's ~70ms for D7. I will try to benchmark with this patch as soon as I have it back.
Comment #141
catchI'd be happier if someone did some before/after profiling of this with xhprof (including a compiler pass so we can see how bad it is) before it goes in. The benchmarks here aren't very useful, but profiling should show what's being saved and we can extrapolate that for when a lot more services are registered. Profiling could be done by me but not likely today.
Comment #142
podaroktagging
Comment #143
effulgentsia CreditAttribution: effulgentsia commentedMacBookPro6,2, i7 dual core 2.66GHz. MAMP 2.0, PHP 5.3.6, APC enabled, XDebug disabled. Benchmarking with
ab -c1 -n100
(i.e., anonymous user, all caches warm (but Drupal page cache not enabled), after a new Standard profile install). And also for reference, I get 30ms on D7 HEAD vs. 50ms on D8 HEAD, so we have some work cut out for us on optimizing D8, but we know that already.Comment #144
pounardOk, thanks, makes a lot of sense you're outrunning me that much :)
Comment #145
BerdirDid some benchmarks too, but the differences between each run were bigger than the improvements from this issue. Page requests take about 45-50ms here (80ms with 10 nodes listed on frontpage, that decrease worries me) with and without the patch.
@effulgentsia Did you run ab multiple times an these numbers were consistent?
As expected, not seeing a difference when page cache is enabled, because this code is never run in that case. What might be problem there is that we will add more and more stuff to the bootstrap container (database, caches, ...), any chance we can improve that too? (later, not in this issue).
Same numbers as @effulgentsia when disabling using the compiled container but always dumping it. However, testing showed that disabling/enabling the watchdog() call didn't really change anything, runs varied between 58 and 61ms, with and without that watchdog call.
Also looked at it with xhprof, but not really sure what exactly I should be looking for, some notes:
- on HEAD, ContainerBuilder::get() takes about 4-5ms (~7%) half of that is spent in createService(), if compiling really saves us most of that, then this would actually be consistent with @effulgentsia's numbers.
- With the patch, Container::get() is around ~2.6ms (~4.5-5ms), more than 50% of that time is spent in getHttpKernelService() and getRouterListenerService(), both require 10x as much time as all other calls. kernel does this: "new \Drupal\Core\HttpKernel($this->get('dispatcher'), $this, $this->get('resolver'))". Which means that it also includes the whole dispatcher stuff, which probably explains this.
- However, note that I'm still seeing 5 calls to ContainerBuilder::get(), which is the bootstrap stuff, that take up ~0.5ms (almost 1,5ms in one of 4 requests, might be a fluke). This will only go up once we add more stuff here, and we'll have to.
- dumpDrupalContainer() takes around 10-11ms (11%) here, 95% of that is in PhpDumper, but this is on a fast SSD, might be slower with a slow disk. Not sure about sites with multiple webservers, in this case, it might be save to have these files in a local file system, but will that be true for all stuff that we will put into the protected directory? If it needs to be shared, could it be that loading it from the network might be slower than building them?
- ContainerBuilder::compile() is another 14ms, but our two current compiler passes are rather irrelevant, it looks like this runs 15 default compiler passes, which make up 90% of the time or something around that. See screenshot in next comment.
@catch: Is that enough data for you?
Edit: Added another note about compile().
Comment #146
BerdirAttaching screenshot for the compile() execution, see above for explanation, can't upload an image to an existing comment.
Comment #147
katbailey CreditAttribution: katbailey commentedWith #1331486: Move module_invoke_*() and friends to an Extensions class this will never happen on a normal page request - at that point it will no longer be a "bootstrap container" but more of a "maintenance-mode container", only used when there is no kernel. And then we'll be more likely to be taking stuff out of it than putting anything else in.
Comment #148
BerdirGot it. Nice.
Also, don't rely too much on the testbot runtime. This seems to vary quite a bit across differrent bots, the latest 8.x run for example took 33minutes and I've also recently seen runs that only took 26 minutes.
I've recently noticed that my new system (i7 quad core 3.5ghz) is able to run the test suite quite fast, so if necessary, I could do a before/after testrun to get some more comparable results. Or parts of it, the differences should be more or less consistent for all test runs I think.
Comment #149
Fabianx CreditAttribution: Fabianx commentedMore benchmarks (XHProf) with 100 nodes and a view-source: on /node | frontpage
BASE: Symfony\Component\DependencyInjection\ContainerBuilder::get - 121-142 ms
ALREADY COMPILED: Symfony\Component\DependencyInjection\Container::get - 57 ms
IN COMPILE STAGE: Symfony\Component\DependencyInjection\ContainerBuilder::compile - 50 ms
IN COMPILE STAGE: Drupal\Core\DrupalKernel::dumpDrupalContainer - 35 ms
( building takes 57 ms, which includes 50 ms of compile time)
Site load matches that:
BASE With builder: 2,086,362
With compiled: 2,009,212 ( around 76 ms faster, 57 + 76 == 133 ms, which is within measured range)
With need to compile then run: 2,223,836 ( == around 137 ms slower than BASE, 90-100 ms is building and dumping in Drupal\Core\DrupalKernel::initializeContainer, the rest seems to be running and measurement tolerance)
--
Before (measured with XHProf on) with page timer: 21[20-60]
After (measured with XHProf on) with page timer: 20[40-80]
Hope that helps.
--
AB Benchmarks:
Compiled DIC
Core
Without XHProf I am also seeing speedup of 10ms, which is around 2%.
( ab -c1 -n100 against 100 nodes on /node )
Tested several times and numbers where totally consistent on runs.
Compiled DIC around 560 (min), Core around 568 (min) and the rest matches that.
Comment #150
effulgentsia CreditAttribution: effulgentsia commentedYes, while any single request can easily vary by many ms, averages of runs of 100 are consistent within 0.2ms.
Sites that scale to multiple dedicated servers will need to decide how to configure their infrastructure, but my guess is that for many, the best choice will be to have compiled PHP storage configured to use the local system of each web server. This means 1 slow request per server after enabling or disabling a module. That's nothing compared to optimizing the millions of requests that follow that. The reason drupal_php_storage() takes a $name argument is so that only dynamic php that can't be easily regenerated per web server needs to be mapped to a networked file system. Neither DIC nor Twig are use cases of that, but if we at some point want to replace Update Manager to use drupal_php_storage() instead of FileTransfer, that might be.
Comment #151
catchThanks for the additional benchmarks and profiling, that's really handy!
So it looks like
- we do get a small performance improvement from compiling, but it's not exciting at all.
- the cost of compiling itself isn't bad at all (nothing compared to a menu/theme/registry/schema rebuild).
I was expecting a bigger performance increase here, so profiled the front page as well. It looks like we still run
Drupal\Core\DrupalKernel::registerBundles
on every request, which took around 3ms that profile run (attached the screenshot).I was under the impression that once we compiled the container, we'd be able to avoid doing that every request. Additionally, if we're still doing it every request, then why do we care if the module list is different in terms of the compiled container? If this patch just enables that, but doesn't actually integrate this bit yet, than that might be fine but could use a follow-up at least. Would be good to understand this a bit better since it seems we're missing out on a 3ms improvement at the moment.
Related to this, the patch is currently relying on the module list changing the unique hash of the dumped container, which means that if you enable or disable a module you'll get a new cache ID and hence new rebuilt container. However I don't see any logic to handle the following:
- a module adds a bundle class that previously didn't have one
- a module removes a bundle class when it previously did have one
- changes to core bundle registrations
I understand there's no garbage collection here yet, but shouldn't there be a line added to drupal_flush_all_caches() to wipe the entire directory or similar?
At the moment a drush cc all or similar won't affect any of these at all. And following on from that, if you do have multiple webservers, a drush cc all (should that force a container rebuild) will only work local to that server - this might be a problem for people with multiple webservers to figure out rather than core but it probably needs documenting.
Comment #152
pounardI have a question thought: does this patch effectively compile the pre-kernel container (the one hardcoded in drupal_container())? Because if not, we are compiling only half of the stuff we could compile.
Comment #153
chx CreditAttribution: chx commentedRe #152, kindly read #147. Thanks!
Comment #154
pounardThe real important point of my semi-question was: we are compiling only half of the stuff we could compile: this just means that benchmarks here are only half-significant here since things are going to be improved later.
Comment #155
chx CreditAttribution: chx commentedNo the point is that you do not read the comment which says the other half is going away.
Edit: how that benchmarks, I guess we will see there. This patch is more held up by the fact that it does not react to code changes than this.
Comment #156
chx CreditAttribution: chx commentedNow, we wipe.
Comment #157
Lars Toomre CreditAttribution: Lars Toomre commentedThere are several documentation and/or coding style issues that need to be addressed in #156. Is this the correct time to roll a patch that incorporates such changes?
Comment #158
xjm@Lars Toomre, if it's covered by http://drupal.org/core-gates#documentation-block-requirements, yes. I didn't spot any missing docblocks, datatypes, etc. on a quick scan. If it's not covered by that (e.g. the
Test
rather thanTests
), please provide the reroll yourself with an interdiff, or file a followup after the patch goes in. This is a critical blocking another critical which is blocking all progress on Views in Core.That said, the change in #157 should probably be reviewed. (It looks fine to me, wiping the compilation on a full cache clear.)
Comment #159
pounardGod, you are just unbelievable. Just stop answering me if that makes you angry.
Comment #160
effulgentsia CreditAttribution: effulgentsia commentedKeep in mind that in addition to pounard's point that this patch doesn't improve the performance of services registered or retrieved during bootstrap (per #147, to be fixed in #1331486: Move module_invoke_*() and friends to an Extensions class), this patch also makes CoreBundle register all the event listeners as services: had those been registered as services in HEAD (even without compiler passes), HEAD would have been slower, so this patch's gains would have been more impressive: we kept those out of uncompiled service registration to minimize the interim performance impact of the earlier WSCCI patches. In general, the way to think of this patch is that it lowers the overhead of DIC services to the point that we can put things in the DIC without worrying about overhead, vs. otherwise having to pay a penalty for every DIC service (details in #1673162: Analyze performance of uncompiled and compiled DI containers).
Yes. The follow up for that is #1716048: Do not boot bundle classes on every request.
#156 handles single server flushing, but doesn't address the multi server case. Can we defer the multi server case to #1759582: Figure out when to delete compiled DIC files? One possibility is adding an extra random component to the hash similar to how we handle cache busting for JS and CSS files, but maybe there's better ideas than that we can explore in the other issue.
Comment #160.0
effulgentsia CreditAttribution: effulgentsia commentedexplanation.
Comment #161
catchOK I spoke to chx in irc. I'd misunderstood the bundle dependency here a bit, we don't ever register a bundle to the DIC, but the bundles themselves can register services which affect the DIC. This is why we can't get rid of the slow class_exists() calls and bundle registration from the patch just by having a compiled DIC :(
It would be good if we could only load the bundles during a compiler pass. I understand that may break things because bundles can have a boot() method which gets called every request, not sure what that's needed for or why we'd need to support it though - if we need a Symfony-esque equivalent to hook_boot() that could be done via EventDispatcher couldn't it? The hard-coded method on the bundle class with an empty implementation feels like a poor regression from the existing hook system. I also couldn't find any examples of where it's supposed to be useful.
We don't necessarily have to fix that here, but if we don't, then I'll want to open a new major task to fix the performance of bundle registration for modules, that 3ms is 3ms too much.
Having figured that out, that means that basing the container on the list of modules enabled isn't going to work, or not by itself, since it doesn't take into account code changes at all.
This is fine as it is, but if you add stuff to your bundle, or update a module that has a bundle that previously didn't, then nothing is going to happen to the compiled container. I think we probably need to fix that before this goes in, or it'll need to be critical bug report if there's something this is absolutely blocking and it turns out to be very hard to get right.
Comment #162
catchMassive cross-post, I haven't reviewed #156 yet.
Comment #163
Crell CreditAttribution: Crell commentedTo call boot() or not to call boot(), that is the question. And it's being asked over here: #1716048: Do not boot bundle classes on every request. Let's punt that to that issue since this issue is part of a knot of bootstrap ugliness spread across several issues.
In Drupal 7 now, adding a new hook requires a cache clear. Adding a new theme function or template requires a cache clear. Adding a new module requires a cache clear. I think I'm missing why for development purposes adding a service to a module requires an explicit cache clear is a problem. Garbage collecting old containers is already punted to #1759582: Figure out when to delete compiled DIC files. Or if you're in active development you use a dev mode that recompiles on every request (like flushing the theme registry on every request).
I guess I'm missing why that's as critical as you're suggesting.
Comment #164
Crell CreditAttribution: Crell commentedSigh.
Comment #165
catch@Crell, the reason why it would be a critical, is because there was no way to clear the cache, nor cache clearing, in the patch. It's OK to have to clear the cache, it's not OK to have a cache that can't be cleared (at least not without manually locating the PHP files on the file system and rm-ing them).
#158 apparently adds that though so needs reviewing for this, I think the CNW was a cross-post.
Comment #166
pounardI think this is not an issue, then answer you are looking for is in question you asked: if we register a compiler pass that effectively register all existing bundles
that implement[EDIT: Sorry, boot() method is in the interface... Sad Symfony is sad, ZF2 actually relies of introspection for this, no interface] into a list as a container paramater, we can then callboot()
boot()
on existing bundles on a normal query (with compiled the DIC) without too much penalty: the list is hardcoded as a bundle parameter and allows us to instanciate those classes(and minimal since there will be only a few classes, since[EDIT: See upper edit] and without usingboot()
is a very uncommon operation to implement)class_exists()
.This is possible because no bundle related operations at all are done after the DIC has been initialized (see the original Symfony Kernel::boot() method).
If Drupal does otherwise, and override the
boot()
method and reverse the order at some point, then we are doing it wrong™. Symfony does build the container before running any bundle operation.EDIT: Anyway, giving that not all modules will implement a bundle, this is still fine to do this, we are not yet at the point where all module will do that, it gives us some time to think, meanwhile, we can implement the compiler pass magic trick.
Comment #167
Fabianx CreditAttribution: Fabianx commented@pounard: Did you meant to change the status or was that xpost again?
Comment #168
pounardOh sorry, cross-post...
Comment #169
effulgentsia CreditAttribution: effulgentsia commentedThe test failures are a problem though, so back to needs work.
Comment #170
chx CreditAttribution: chx commentedBlargh.
Comment #172
chx CreditAttribution: chx commentedComment #173
catchSo if we just delete all the files when doing this, then is it necessary to add the system_list hash to the cid then? It makes me very uncomfortable that figuring out which DIC to use requires having the system list available. Seems like the comment relating to testing is no longer true either.
Comment #174
pounardEDIT: Stupid comment.
Comment #175
catchYes since the latest patch flushes it manually, I don't think we need either.
Comment #176
pounardOups you were faster than me.
Comment #177
no_commit_credit CreditAttribution: no_commit_credit commentedComment #178
xjm#177 contains documentation style nitpicks:
There are two things that need updated documentation, though. The
system_list()
docblock needs to be updated to explain the new behavior (update the list for$type
, update to describe both possible return values, and explain more about the new behavior that's added). Also, I punted on the summary for theDrupalKernel
constructor; that docblock could explain more what we're doing in the overridden constructor.Two typos I missed:
Missed this on the first pass, but there's another typo here ("servies") and the second word "services" should be removed.
That semicolon was supposed to be a period.
Comment #179
effulgentsia CreditAttribution: effulgentsia commentedSuppose a use case where someone wants to use FileReadOnlyStorage as the implementation on a production site, but still allow some modules to be enabled or disabled on prod, and therefore wants to place multiple precompiled DICs into the storage bin.
Comment #180
catchIs that a valid use case though? I'm not sure it justifies the dependency on the module list at all.
Comment #181
effulgentsia CreditAttribution: effulgentsia commentedIn the interest of moving this along and unblocking #1708692: Bundle services aren't available in the request that enables the module, I'd be fine with removing the system_list hash and deferring discussion of #179 to #1759582: Figure out when to delete compiled DIC files.
Comment #182
effulgentsia CreditAttribution: effulgentsia commentedOh, wait. One reason for the hash is related to #1708692: Bundle services aren't available in the request that enables the module. If in the same PHP request, we need a new DIC, then it also needs a new class name, since PHP can't dump a class from memory and reload new code for it.
Comment #183
pounardTrying to build a new container en route seems conceptually wrong somehow. But that belongs to the other issue.
Comment #184
effulgentsia CreditAttribution: effulgentsia commented@catch: what's your resistance to the system_list() dependency? If the compiled container doesn't exist, then DrupalKernel needs to iterate the enabled modules in order to build the container. Since DrupalKernel needs to be able to initializeContainer() regardless of whether the compiled container already exists, it already needs a way to access system_list. If the objection is just one of performance rather than decoupling, would it be okay to figure out how to optimize the system_list() away in a follow up? We could, for example, name the DIC class based on the hash of its contents (which would remove the need for deleting in response to a code update), but then we'd need a state() variable to let DrupalKernel know the current DIC name, and I'm not sure that a state() dependency is any better than a system_list() dependency.
Comment #185
effulgentsia CreditAttribution: effulgentsia commentedHere's an idea: we can hash on DIC contents and maintain the value of that hash in cache('bootstrap'). It doesn't need to be state(), since losing it is no big deal, it just means we need to spend a bit of time rebuilding. And meanwhile, we're pretty much guaranteed to have already needed cache('bootstrap') by the time we instantiate DrupalKernel. I'll try this out and post a patch. Ping me in IRC if you want to dispute this before seeing the patch.
Comment #186
effulgentsia CreditAttribution: effulgentsia commentedThis implements #185, reverting the need for system_list() to store a hash or support a null $type.
Comment #187
effulgentsia CreditAttribution: effulgentsia commentedFixes a PHPDoc.
Comment #188
effulgentsia CreditAttribution: effulgentsia commentedIn IRC, chx said that if we're reverting some system_list() changes, we can revert all of them, so this does that. Also, this adds an index.php change which I mistakenly left out of prior 2 patches.
Comment #189
effulgentsia CreditAttribution: effulgentsia commentedSince there's been a lot of small rerolls since #124, roughly the last patch that was substantively RTBC'd, here's the full interdiff from that to #188.
Comment #190
Crell CreditAttribution: Crell commentedThat's only true now. That has a high likelihood of changing, especially with #1331486: Move module_invoke_*() and friends to an Extensions class.
Comment #191
effulgentsia CreditAttribution: effulgentsia commentedGood point. But even in that issue, ExtensionHandler::__construct() requires $bootstrapCache. However, if we want to get to a point where DrupalKernel::initializeContainer() can run with neither an $extensionHandler nor a $cacheBackend, then I'm at a loss for anything other than a hard-coded class name for the DIC. For that, we can get around #182, by making DrupalKernel always use an uncompiled container if it detects that the compiled class already exists in memory (and is therefore suspect), but then we'll still have the problem that a cache clear would need to delete the compiled DIC from all web servers, since a DrupalKernel::initializeContainer() call on all other web servers would have no persistent information in Drupal that it could access to know that its local compiled container is stale. Given that, my suggestion is to proceed with #188's approach of allowing DrupalKernel::__construct() to receive a cache backend object, and bang our head on how to decouple that away in a follow up. But I'm open to other ideas if anyone has any.
Comment #192
chx CreditAttribution: chx commented#191 is reasonable. This is a major feature that needs to get in ASAP and then minor tuning can happen past Dec 1 should we find we do not want a bootstrap cache any more. But, given how even the new system_list rides on that and how much Drupal depends still on modules, I do not see a chance of bootstrap cache dying.
Comment #193
Crell CreditAttribution: Crell commentedHrm. Yuck. I'm OK with punting on that if we drop a @todo in the appropriate place so we are less likely to forget.
My only other idea is to make Extension a requirement (likely in the other issue) and have some extra value in the array so that a no-module situation still results in a key of some sort. But that can be done later.
Comment #194
catch@effulgentsia I'm uncomfortable with the system list dependency for the following reasons:
1. It doesn't actually do what it says (container contents can change even if module list is the same, or they might not change with a different module list)
2. It introduces a hard dependency on the Extensions class (when we have it), which is a fairly high-level concept in Drupal compared to most things registered to the DIC.
3. It just feels bad.
The hash of the contents doesn't have any of these problems though. This also ought to fix the following without depending quite so much on the explicit clear.
Enable a module that provides a bundle. (hash 1)
Disable the module (hash 2)
Remove the bundle from the disabled module.
Enable the module again (back to hash 1, but in this case a different container).
Comment #195
chx CreditAttribution: chx commentedThen this is good to go. #194 says that the hash approach in #188 is good if I read it correctly.
Comment #196
xjmThe
@todo
in this went away, but the docs for $environment and $bool still say only one value is allowed. Is it still accurate?Meanwhile, rerolling to cleanup the typos mentioned in #178.
Comment #197
no_commit_credit CreditAttribution: no_commit_credit commentedComment #198
RobLoachRTBC++... Would love to get this in soon as we could then move a lot of the service registrations that are directly in
->build()
over to use Compiler Pass to better take advantage of compilation.Comment #199
catchCommitted/pushed #197, thanks all!
Comment #200.0
(not verified) CreditAttribution: commentedUpdated issue summary.