Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APC CSS and JS check #4

Merged
merged 2 commits into from Mar 9, 2012
Merged

APC CSS and JS check #4

merged 2 commits into from Mar 9, 2012

Conversation

@davidstrauss
Copy link
Member

@davidstrauss davidstrauss commented Feb 14, 2012

No description provided.

davidstrauss added a commit that referenced this pull request Mar 9, 2012
@davidstrauss davidstrauss merged commit 66830fc into master Mar 9, 2012
@chx
Copy link

@chx chx commented Feb 12, 2013

This makes my laptop hang. If I comment out the apc_store then it works. You can find a phpinfo at http://drupal4hu.com/i.html

@catch56
Copy link

@catch56 catch56 commented Feb 12, 2013

Also http://drupal.org/project/agrcache stops the file_exists() from executing altogether without a core hack, and solves several other performance issues when css and js aggregates have to be generated.

@joshkoenig
Copy link
Member

@joshkoenig joshkoenig commented Feb 12, 2013

@chx - A subsequent check should have added a check to insure that apc is enabled. See the current state of master:

https://github.com/pantheon-systems/drops-7/blob/master/includes/common.inc#L4871

@catch56 - We need this check in DROPs because we can't require people use certain contribs and the impact on the average site using a network file system is non-trivial.

@chx
Copy link

@chx chx commented Feb 12, 2013

apc is enabled alright, that's why it hangs on the apc_store call. You can see the phpinfo I linked that my apc is enabled.

@davidstrauss
Copy link
Member Author

@davidstrauss davidstrauss commented Feb 12, 2013

@catch56 Having a module that exploits hooks to "patch" core is worse than just patching core. You end up duplicating a bunch of code, obfuscating the actual change, and increasing the chance of missing an upstream bug or security fix.

@chx
Copy link

@chx chx commented Feb 12, 2013

--enable-apc-pthreadrwlocks solves the APC hang (HT Rasmus)

On Tue, Feb 12, 2013 at 11:22 AM, David Strauss notifications@github.comwrote:

Having a method that exploits hooks to "patch" core is worse than just
patching core. You end up duplicating a bunch of code, obfuscating the
actual change, and increasing the chance of missing an upstream bug or
security fix.


Reply to this email directly or view it on GitHubhttps://github.com//pull/4#issuecomment-13450687.

@davidstrauss
Copy link
Member Author

@davidstrauss davidstrauss commented Feb 12, 2013

--enable-apc-pthreadrwlocks solves the APC hang (HT Rasmus)

Sounds like a bug in APC.

@catch56
Copy link

@catch56 catch56 commented Feb 12, 2013

There's a core patch at http://drupal.org/node/1014086
On 12 Feb 2013 19:30, "David Strauss" notifications@github.com wrote:

--enable-apc-pthreadrwlocks solves the APC hang (HT Rasmus)

Sounds like a bug in APC.


Reply to this email directly or view it on GitHubhttps://github.com//pull/4#issuecomment-13451273.

jsacksick referenced this pull request in commerceguys/kickstart-drops-7 May 4, 2014
Make it possible to install Kickstart without DFP.
angeloporretta pushed a commit to angeloporretta/drops-7 that referenced this pull request Oct 9, 2014
@btopro
Copy link

@btopro btopro commented Oct 16, 2014

Is there a reason not to have this patch live on d.o. in a thread so that it can be included in performance optimized drupal distros? currently d.o. packager wouldn't be able to pick this up and if there's no objection to having it on d.o. I was going to open an issue just to get the patch there / increase visibility.

@davidstrauss
Copy link
Member Author

@davidstrauss davidstrauss commented Oct 16, 2014

It's a good general-purpose patch, and I know folks outside of Pantheon use it effectively.

@btopro
Copy link

@btopro btopro commented Oct 18, 2014

Cool @davidstrauss thx for the notice. A coworker and I are thinking of taking the performance enhancements present in ELMSLN and rolling them into a fork of Pressflow for more general usage. After reviewing the Pressflow changes this probably makes more sense then pulling in this 1 enhancement alone :)

@nstielau
Copy link
Contributor

@nstielau nstielau commented Oct 20, 2014

Awesome @btopro, keep us up to date on the progress!

@btopro
Copy link

@btopro btopro commented Oct 20, 2014

Ok, did some metrics, lots of A/B/C/D comparison and then I published the results to the repo as well.
https://github.com/btopro/presserflow7

https://github.com/btopro/presserflow7/tree/master/_PATCHES -- has a list of all the patches applied. These are all available on Drupal.org. The PATCHES.txt file also includes some of what was used in the settings.php for the APC / "kitchen sink" cache bin / contrib tests.

https://github.com/btopro/presserflow7/tree/master/_RECIPES -- has a drush recipe that can be applied to any site to get it up to kitchen sink status / settings for comparison of the metrics (modules / variables applied).

https://github.com/btopro/presserflow7/tree/master/_METRICS -- xls sheet that has the raw values, some of the methodology, etc.

This was tested against three testbed sites so it should get some independent / additional validation but results were promising; needs more testing but at the very least it's a good series of patches for you all to check out.

@davidstrauss
Copy link
Member Author

@davidstrauss davidstrauss commented Oct 20, 2014

Hi, @btopro. Your changes look great. We should consider rolling more of them into Pressflow 7.

Also, while you're welcome to create your own fork, please choose a name more distinct than "Presserflow."

@btopro
Copy link

@btopro btopro commented Oct 20, 2014

lol yeah that was just for the experiment; sorry. I made it more obvious since this is mostly for getting reaction to try and get some of these rolled into pressflow (where they make sense) and a blog post on the methodology -- https://github.com/btopro/Presser-Flow-FORK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.