Doesn't have to be ten but it would be good to collate some of the bad ones in one place.

This will be useful for:
- writing test cases - we can simulate the regression / revert patches to see if automated testing picks it up. Also we might want to test specific scenarios that would have caught things if they're major (like page caching).

- documentation for http://groups.drupal.org/node/158769

- I've had it in mind to do a "what not to do" blog series or something a bit like coder tough love, but need to collect ammunition in one place first.

More or less off the top of my head, please add more as you think of them:

#978144: cache_get_multiple() inconsistent with cache_get() - cache_get() was changed to wrap cache_get_multiple(), this switched a db_query() for db_select() and led to #1064212: Page caching performance has regressed by 30-40%. Original issue (writer shall remain nameless) specifically stated there was no chance of performance regression "it's not like this is a cheap function and that the additional call could possibly matter".

#623992: Reduce {system} database hits - page caching again, this time a 50% regression. Patch was committed, was a bit broken, followup patch introduced the performance regression. This would also have affected memory usage.

#1061924: system_list() memory usage - memory, regression was introduced by #887870: Make system_list() return full module records, ab benchmarks were done on the original patch (at my request), but no-one discussed the memory issue. Second major regression from abuse of system_list() to load stuff not necessary for boostrap :(

#1014130: install_profile_info() does a file system scan on every request to admin/config (and etc.). Introduced by #509398: Install profiles should be modules with full access to the Drupal API and all it entails(.install files, dependencies, update_x). File system i/o and CPU. /admin only, and only with non-default install profile. Original discussion had no performance tests at all.

#372743: Body and teaser as fields - this one we knew about before it went in. Big CPU hit on node rendering.

Comments

moshe weitzman’s picture

moshe weitzman commented

The biggest abomination I can recall was #331611: Add a poormanscron-like feature to core which initially added a second full bootstrap for every request. A load doubler! Long fixed.

There were some major problems around dashboard, shortcuts, and overlay when they first landed but I don't recall specifically.

catch’s picture

catch
he/him
Primary language English
commented

Ooh poormanscron is a good one, I'd almost repressed the memory. It was a full bootstrap added to cached pages too :(

Contextual links was quite a big hit, that was a completely new feature though so there wasn't a single regression, more a lot of overhead added by the feature in general and very variable depending on the page (comment listings were the worst). Automated testing of the standard profile with admin users might have caught that.

Overlay as far as I know was all front end issues performance - mainly page rendering/redrawing this kind of thing. Need browser instrumentation for page rendering for that (and again admin tests). A very good front end example is #1203766: With large number of permissions /admin/people/permissions becomes unusable - only just found and fixed.

That reminds me of #296693: Restrict access to empty top level administration pages - that fix was a serious regression, but it became most obvious when combined with the toolbar - since we were then rendering links to specific admin blocks on every request.

yched’s picture

yched commented

#372743: Body and teaser as fields - this one we knew about before it went in. Big CPU hit on node rendering.

Field rendering got optimized later in #652246: Optimize theme('field') and use it for comment body (with final benchmarks in #75 over there). I can't remember of any "pre body-as-field vs. post theme('field')-optimisation" benchmarks, but I'm not sure the overhead is so massive in the end.

catch’s picture

catch
he/him
Primary language English
commented

Right this isn't so much about regressions that were never fixed, it's more patches that were committed that caused a measurable performance regression when they went in. If and when this gets written up properly we should definitely link to the issues where performance was improved again though.

catch’s picture

catch
he/him
Primary language English
commented

Here's a couple of overlay ones #615130: Overlay locks up the browser and consumes 100% of CPU for certain browsers/graphics cards/operating systems [#704182[, this was more 'newly introduced performance issue via a feature' or alternatively "feature as performance regression". Front-end only.

Shortcuts: #620634: Shortcuts are built unconditionally for anonymous users- this was a straight regression just from having the module enabled so we should have caught that prior to commit.