New menu system: faster and less memory

Nice roadmap. Looking forward to this.

Subscribing.

Of course, I have at least the ideas (well, some code too) to get all four

Could you post some of the code, even if not in the form of a patch? I'm curious as to how this will work.

It's still difficult to grok the semantic difference between _boot() and _init(). Not sure it can be made clearer but I can see this being a source of confusion.

+ * To generate these, we will use binary numbers where bit 1 represents
+ * original value and bit 0 means wildcard. If the path is node/12/edit/foo
+ * then the 110 bitstring represents node/%/edit where % means that any
+ * argument matches that part.

I'd write 'value 1' and 'value 0'. When you say 'bit 1', it makes me think of the bit at position 1.

In the example, what happens with 'foo'? The original path consists out of 4 parts, but the pattern only has 3. This needs to be clarified in the code comments. No need to clarify it in the issue.

There are some typos in the following snippet (i.e. 'will is an integer). Also, it is not quite clear what you're trying to say. The documentation describes the implementation details but not the bigger picture? I.e. why do we want to unserialize an array in an awkward way? Needs to be documented in the code. :-)

+ * Unserializes an array. Integer values are mapped according to the $map
+ * parameter. For example, if unserialize($data) is array('node_load', 1)
+ * and $map is array('node', '12345') then 'node_load' will not be changed
+ * because it is not an integer, but 1 will is an integer. As $map[1] is
+ * '12345', 1 will be replaced with '12345'. So the result will be
+ * array('node_load', '12345').

Subscribing...

Subscribing (I think its time we have a subscribe feature in D6)

subscribing

This breaks the system_get_module_admin_tasks function which still expects the old ['path index'] key from menu_get_menu. It will need to be rewritten. The effect is that the page q=admin/by-module has no module specific links.

Another problem in system.module, system_main_admin_page seems to return halfway through, causing q=admin to white-screen.

Perhaps this is intentional and both the admin and admin/by-module above are waiting for the core of the menu system to stabalize - I'm unsure.

I really need to provide more context in IRC.

I was talking about my little test module which was thoroughly tested. That is the part I found to be solid, obviously not the admin stuff ;)

you included your settings.php

node/add/page and node/add/story both return "page not found" errors for me.

Hmm, I logged out and logged back in and it worked. Maybe it was a caching issue.

I did notice that if you go to /user when logged out it doesn't display the login form, but instead tries to display a user profile ("member for 37 years 3 weeks").

watchdog_help was lost due to collateral damage.

A basic benchmark. prepatch has all blocks disabled.

Only default core modules enabled. 

node/!/edit is a non existing node (a different 404).

time (ms)				
            pre     post    +/-
node/1      230.72  179.25  51.47
nopage      221.50  165.63  55.88
node/1/edit 225.00  173.66  51.34
node/!/edit 236.50  169.31  67.19
				
			
# req/s		
            pre   post  +/-
node/1      4.33  5.58  1.25
nopage      4.51  6.04  1.53
node/1/edit 4.44  5.76  1.32
node/!/edit 4.23  5.91  1.68
				
				
All core modules - statistics, menu			
				
time (ms)
            pre     post    +/-
node/1      305.28  248.84  56.44
nopage      283.50  230.56  52.94
node/1/edit 290.22  241.69  48.53
node/!/edit 289.38  234.53  54.84
				
# req/s		
             pre   post  +/-
node/1       3.28  4.02  0.74
nopage       3.53  4.34  0.81
node/1/edit  3.45  4.14  0.69
node/!/edit  3.46  4.26  0.80

Heine, how do I need to read/interprete these results? What format are they in?

I gave this patch a decent workout tonight. My first impression is bliss - these are the fastest pages i've ever served on my dev box. It is magical to see a single simple query retrieve the active callback. For node/1, that query is SELECT * FROM menu WHERE path IN ('node/1','node/%','node') ORDER BY fit DESC LIMIT 0, 1

the menu cache is gone. hook_menu_alter() is introduced. there is a lot to love here.

code-wise, looks good. i think modules will easily convert to this scheme. there is some binary magic which i don't understand but chx and dries do, so thats all good.

there is some debugging in the patch, and some pages which await rewrite. i think thats OK, but i'll wait and see where chx intends to leave off for this phase 1. from my perspective, this is close to rtbc.

I think maybe Heine meant:

            (ms)                       (requests/second)
            pre    post   change       pre  post change
node/1      230.72 179.25 -51.47 (22%) 4.33 5.58 +1.25 (29%)
nopage      221.50 165.63 -55.88 (25%) 4.51 6.04 +1.53 (34%)
node/1/edit 225.00 173.66 -51.34 (23%) 4.44 5.76 +1.32 (30%)
node/!/edit 236.50 169.31 -67.19 (28%) 4.23 5.91 +1.68 (40%)

and

            (ms)                       (requests/second)
            pre    post   change       pre  post change
node/1      305.28 248.84 -56.44 (18%) 3.28 4.02 +0.74 (23%)
nopage      283.50 230.56 -52.94 (19%) 3.53 4.34 +0.81 (23%)
node/1/edit 290.22 241.69 -48.53 (17%) 3.45 4.14 +0.69 (20%)
node/!/edit 289.38 234.53 -54.84 (19%) 3.46 4.26 +0.80 (23%)

... where 'pre' is 'pre-patch', 'post' is 'post-patch', and the first dataset is with only the default enabled core modules.

Please correct me if I'm wrong! I calculated percentages to compare—if it's really at least 20% decrease in page time / request rate, that's very snappy.

Wow, impressive benchmark results :)

I haven't tested this patch yet, but I've read the whole thing, and it looks really seriously amazing. You're a genius, chx! (but hey, we already knew that).

My biggest concern is that there doesn't seem to be proper support for non-integer wildcards in menu paths. E.g. in the patched version of node_menu(), there should just be one path for all the custom node type editing pages: admin/content/types/%. But instead, there's a foreach loop that's defining a path for each node type. This is not good: we need to be able to use strings as wildcards as well! I'm sure there will be plenty of other places, in core and contrib, where strings will need to be used as wildcards.

The binary tricks in menu_get_ancestors() are all voodoo to me, but if they work as advertised, then I say bring it on!!

Good work chx.

Subscribing.

OK, I have spoken to chx on IRC, and in case anyone else reads the patch and worries like I did, please rest assured:

The is_int() check in menu_unserialize() has nothing to do with wildcards. It is perfectly possible to have non-integer wildcards with this patch.

BTW, I did actually read http://en.wikipedia.org/wiki/Bitwise_operation while I was looking at the patch, and it helped a lot. The operators themselves are not voodoo, it's just applying them in code that seems like voodoo to a lot of people (including myself).

subscribing

Fixed a few minor coding style errors, added a few basic doxygen comments, changed home to Home in the breadcrumb placeholder.

The issue with "create content" (a MENU_GROUP_ITEM) showing when no items are available can be tackled post stage 3.

From #drupal:

<chx> with the latest code reorganization there could be something
<chx> ChrisKennedy: this needs to wait until post phase 3. MENU_GROUP_ITEM needs to be handled almost like an expanded item just that the expanded items are not rendered but each and every needs to be inspected. "

verified that the most recent patch works as advertised. RTBC. later phases will happen later.