chore(views): refactor views service to store absolute paths #8390
Conversation
ewinslow
force-pushed
the
views-b6844-master
branch
from
6231669
to
f71b40f
Compare
| $params = array('view' => "$view", 'vars' => $vars, 'viewtype' => "$viewtype"); | ||
| $content = $this->hooks->trigger('view', "$view", $params, $content); | ||
|
|
||
| // TODO: Remove backward compatibility with less granular hook in 2.0 |
|
This looks great but feels like it would eat memory/perf. It's too bad we allow view configuration to be completely malleable at all times. I'll dig into it more a bit later. |
ewinslow
force-pushed
the
views-b6844-master
branch
from
f71b40f
to
7f4a147
Compare
|
Yea... I just figure we can always optimize later if that actually becomes an issue. I would like to make our views system more immutable. So you can't keep registering or changing views after the initial boot. |
ewinslow
force-pushed
the
views-b6844-master
branch
from
7f4a147
to
a385480
Compare
|
Latest push fixes some end-to-end issues. I haven't tested performance or whether this is getting cached or anything like that. I'm a little rushed because I'm strapped for time and this is blocking other significant improvements. I do not plan on finishing implementing the incomplete tests before merging this. There are quite a few left but I already implemented way more than we had before so I'm feeling pretty good about that. |
ewinslow
force-pushed
the
views-b6844-master
branch
from
a385480
to
386232f
Compare
|
Worth a note here that this blows up and turns off htmlawed. Also |
|
Will be sending you fixes... |
|
Thanks. |
ewinslow
force-pushed
the
views-b6844-master
branch
from
386232f
to
a9c31db
Compare
|
Some of these issues were caused by bad interactions with the pagesetup event. |
|
Moving some handlers out of the pagesetup event fixes things. But others breaks things. I suspect this has to do with priority. I don't understand the order of things here very well... |
ewinslow
force-pushed
the
views-b6844-master
branch
2 times, most recently
from
2faf1e3
to
5b025e1
Compare
|
OK, figured out why menus weren't registering -- had change the pagesetup,system event guard logic a bit. It's fixed now. Just have to look into htmlawed and dev inspector and we should be golden |
|
htmlawed seems to be working fine after my latest changes. System cache is not going to work anymore for this, so I've removed it. Need to figure out a good way to turn some kind of caching back on so that building up this structure isn't so expensive on each engine boot... would it be horrible to pull this in before we've figured that out? |
ewinslow
force-pushed
the
views-b6844-master
branch
from
5b025e1
to
04df1d4
Compare
|
I'd prefer we didn't. |
|
an overhaul of this size needs a lot more eyeballs on it and to make views even slower would quickly outweigh the benefits this brings. |
|
Brainstorming: views with custom locations are likely to be rare; most views will be in core folder; most requests only use "default" viewtype (maybe separate services and caches); with cache on a lot of view operations should be noop (maybe we use a different views service?). Requiring individual view objects doesn't buy much IMO. Maybe as a nicer API when building up the definitions, but on most requests I don't think we'll want the overhead. |
|
Being able to shard the cache by viewtype and maybe view name prefix I think could be a huge win. How often do we need css/js views, e.g. |
|
I will keep at it. |
|
So the latest iteration of this that I'm almost done with will lazily instantiate View instances. That is, only views which are actually referenced in some way (to be rendered, extended, inspected, etc.) will be instantiated. I think this sufficiently addresses the perf concerns of generating all view instances on every request... This also makes it possible to get path caching back, which is great, though I haven't gotten as far as thinking about how sharding will work. |
|
Sounds great! |
ewinslow
force-pushed
the
views-b6844-master
branch
from
04df1d4
to
0a95202
Compare
|
OK, the code is in. I would guess I've spent north of 20 hours on this at this point... >_<. I apologize for the ginormousness of the change. For my sanity please try to focus only on deal-breakers so I can just address the most important things. E.g. I'd prefer to avoid the busywork of breaking down this PR into smaller ones if it isn't completely required. Note that caching and inspector are still broken, so those are known issues... |
|
FYI default view counts (all approx): 860 total. 200 |
|
~1000 views? I forgot how to count that low. (Seems like not enough to be worth any custom-designed memory optimizations we were discussing before). Wonder how far we could get with a trivial caching solution courtesy of Stash. |
| } | ||
|
|
||
| /** | ||
| * Configure a fallback for this viewtype. |
There was a problem hiding this comment.
I'm lobbying for this feature to go away.
|
|
|
Elgg\Filesystem\GaufretteDirectory::getFiles() is super slow. It's digging through all of |
|
Closing in favor of #8509 |
ewinslow
force-pushed
the
views-b6844-master
branch
from
0a95202
to
fcd5b70
Compare
|
One problem here is that extensions are rendered with all hooks and their own extension. |
|
They are not rendered with all hooks, so far as I can tell, but they are rendered with their own extensions, yes. |
|
Sadly I will not be working on this further. Would have been nice to get this in but alas I wasted my time :( |
|
Not a waste! Spurred a bunch of good changes and we've cleared the path for some flavor of refactoring in 2.x. |
Replaces #7070