for quite big $db_prefix arrays db prefixing can take some time and so decrease performance.

e.g. i 've tested an $db_prefix arrray with ~25entries, which lead to about 20ms more page generation time on /admin (200ms instead of 180).

the attached patch changes db_prefix_tables() to use preg_replace, which seems to be a bit faster (got 193ms instead of 200ms).
further this patch changes matching, so that e.g. profile would match to {profile_fields} and {profile_values}. this allows reducing the size of $db_prefix substantially, think of e.g. the flexinode tables. it's possible to avoid this match by appending a '}', e.g. using 'users}' would match only to 'users' and not to 'users_roles'

so in my use case, i was able to reduce the /admin page generation time to 185ms with an equal prefixing.

CommentFileSizeAuthor
#15 db_prefix-before.png99 KBmrfelton
#15 db_prefix-after.png97.36 KBmrfelton
#15 42827-db-prefix-performance-v2.patch2.58 KBmrfelton
#6 db_prefix3.patch2.9 KBfago
#2 db_prefix2.patch2.81 KBfago
db_prefix.patch1.23 KBfago

Comments

dries’s picture

dries commented
Status: Needs review » Needs work

Looks cool but coding style needs work. It's also a good idea to document the new features.

fago’s picture

fago
Primary language German
Location Vienna
commented
StatusFileSize
new db_prefix2.patch2.81 KB

i've attached an updated patch.

it includes some documentation in the default settings.php file.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs work » Needs review
killes@www.drop.org’s picture

killes@www.drop.org commented

I am wondering whether we could make $search and $replace static variables so we only need to compute them once per page view.

fago’s picture

fago
Primary language German
Location Vienna
commented

good idea, i'll try it

fago’s picture

fago
Primary language German
Location Vienna
commented
StatusFileSize
new db_prefix3.patch2.9 KB

this seems to be another fine improvement.

now, i get only 182-183ms on /admin for my test case.

drumm’s picture

drumm
he/him
Location NY, US
commented
Category: task » feature
coreb’s picture

coreb commented
Version: x.y.z » 6.x-dev

(Moving x.y.z version to a real version number) Feature Request => 6.x-dev

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work

Does not apply.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Version: 6.x-dev » 7.x-dev

Need to move this to the D7 queue.

tisho-1’s picture

tisho-1 commented

This patch has been a lifesaver for me. With 160 shared tables I saw a *** 1 second *** overhead! After applying the patch this overhead is gone.

You can see more information on the problem in this thread: http://groups.drupal.org/node/18369

I hope this makes it at least into 7.x!

Thank you!

catch’s picture

catch
he/him
Primary language English
commented
Category: feature » task

Changing to task and subscribing.

mrfelton’s picture

mrfelton commented

This patch is a must if you're using civicrm which required that you db_prefix over 125 tables to get views integration.

mrfelton’s picture

mrfelton commented
Issue tags: +Performance

Adding 'Performance' tag.

mrfelton’s picture

mrfelton commented
StatusFileSize
new 42827-db-prefix-performance-v2.patch2.58 KB
new db_prefix-after.png97.36 KB
new db_prefix-before.png99 KB

I just benchmarked the db_prefix_tables() this using xdebug and webgrind with a setup using 125 prefixed tables (due to civicrm)

BEFORE
Invocation count: 457ms
Total self cost: 961ms
Total inclusive cost: 1339ms

AFTER
Invocation count: 456ms
Total self cost: 40ms
Total inclusive cost: 79ms

The db_prefix_tables() function went from being THE most expensive function that took up almost 20% of processing time, down, to the 25th most expensive function taking up less than 2% of the total time.

See attached screenshots

Also attached is an updated version of the patch that was created against Druapl 6.14

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical

db_prefix_tables() doesn't exist in D7. I'm not sure if the D7 implementation suffers from any of the same issues, but that's being refactored over at #302327: Support cross-schema/database prefixing like we claim to.

So I'm moving this back to D6. Also bumping to critical since 1/5 of request time is massive.

mrfelton - could you post benchmarks?

catch’s picture

catch
he/him
Primary language English
commented
Version: 7.x-dev » 6.x-dev
mrfelton’s picture

mrfelton commented

@catch: What kind of benchmarks do you want to see? Do the screenshots above not show enough?

catch’s picture

catch
he/him
Primary language English
commented

My bad, I completely ignored the screenshots :(

mrfelton’s picture

mrfelton commented

any movement here - would sure be great to see this fixed in core. The performance hit incurred here is seriously large.

jonne_jvl’s picture

jonne_jvl commented
Status: Needs work » Needs review

Tried it on a 6.17 multisite setup.
Works fine, similar speedup as reported in #1 of around ~20ms

I dont read regex, sorry.

Status: Needs review » Needs work

The last submitted patch, 42827-db-prefix-performance-v2.patch, failed testing.

dpearcefl’s picture

dpearcefl commented
Status: Needs work » Postponed (maintainer needs more info)

If this is still needed, please submit the patch with fixes.

dpearcefl’s picture

dpearcefl commented
Status: Postponed (maintainer needs more info) » Needs work
lyricnz’s picture

lyricnz commented

The strtr/str_replace issue was recently fixed in D7/D8 over on #561422: Replace strtr() with str_replace() for db prefixing. I think that solution removes the need to change the meaning of the tablenames in $db_prefix? If others agree, suggest closing this as duplicate (of the "needs backport" over there).

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Closed (duplicate)

Latest measurements #561422: Replace strtr() with str_replace() for db prefixing