Here is a proposal that could shave off some memory usage in drupal_parse_info_format:

- $constants = get_defined_constants();
+ static $constants;
+ if(!isset($constants)) {
+   $constants = get_defined_constants();
+ }

On my test site (copy of a production site with quite a few modules), get_defined_constants was being called 284 times when loading the admin/modules page, and usually uses more than 128MB memory. Now only once. Uses 30MB less memory now.

This almost seems too simple... What could go wrong here?

Files: 
CommentFileSizeAuthor
#6 xhprof-defined-info2.png116.3 KBquicksketch
#5 parse_info_format-2146643-5.patch769 bytesquicksketch
PASSED: [[SimpleTest]]: [MySQL] 41,093 pass(es). View
#2 parse_info_format-2146643.patch735 bytesquicksketch
FAILED: [[SimpleTest]]: [MySQL] 41,086 pass(es), 0 fail(s), and 1 exception(s). View
#2 xhprof-defined-info.png123.01 KBquicksketch

Comments

quicksketch’s picture

This almost seems too simple... What could go wrong here?

If additional constants become available after the first call to drupal_parse_info_format(), they won't get picked up.

However, 99% of the time, the ONLY constant in a .info file is the VERSION constant used by core. So it's unlikely that this patch would cause a problem.

quicksketch’s picture

Title: Improvement on memory usage in drupal_parse_info_format » Speed up drupal_parse_info_format() 3x and reduce memory 95%
Version: 7.24 » 7.34
Status: Active » Needs review
FileSize
123.01 KB
735 bytes
FAILED: [[SimpleTest]]: [MySQL] 41,086 pass(es), 0 fail(s), and 1 exception(s). View

Here's a patch that eliminates use of get_defined_constants() and instead uses defined() to check if a constant is in use. The results are pretty dramatic:

A 3x improvement in speed and 95% less memory usage.

I also tried a version where I used preg_match() to check if the value was likely to be a constant by checking against /^[A-Z0-9_]+$/, but the overhead of preg_match() seems to be slightly higher than using defined(), even on a long string like a description. So this patch appears to be both simpler and faster than attempting any kind of pattern matching, and it's 100% compatible with the existing approach.

I hope it's okay if I change the title to match, it's more likely to get attention that way. :)

Status: Needs review » Needs work

The last submitted patch, 2: parse_info_format-2146643.patch, failed testing.

quicksketch’s picture

Hm, not sure why that's causing test failures.

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest104843registry' doesn't exist: SELECT filename FROM {registry} WHERE name = :name AND type = :type; Array ( [:name] => dummyClassName [:type] => interface ) in _registry_check_code() (line 3168 of /var/lib/drupaltestbot/sites/default/files/checkout/includes/bootstrap.inc).

That seems odd that use of defined() or constant() would trigger a class name lookup. I'll run this locally and see what the problem is.

quicksketch’s picture

FileSize
769 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,093 pass(es). View

Oh cripes, we have a test for this, defining a .info line as:

; After parsing the .info file, 'double_colon' should hold the literal value.
; Parsing should not throw a fatal error or try to access a class constant.
double_colon = dummyClassName::

Very tricky. ;)

So this triggers a lookup of the class dummyClassName. That's just crazy. If we don't support looking up class-based constants, the regex version of the patch is more appropriate, which ensures it's just a "normal" constant in the global namespace.

quicksketch’s picture

FileSize
116.3 KB

Results on this approach:

Still roughly a 3x increase in speed (2.58x in this run). 95% memory saving stays in place.

quicksketch’s picture

Status: Needs work » Needs review
quicksketch’s picture

joseph.olstad’s picture

Great work @Quicksketch once again you've elegantly improved D7 core.

I'm going to add this to the performance group page.

mikeytown2’s picture

Version: 7.34 » 7.x-dev
Status: Needs review » Reviewed & tested by the community

Looks good. Going to mark this RTBC

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.40 release notes

Very nice. I've looked at this carefully and can't think of a way this would break. For reference, the previous issue that added this originally (to deal with the "::" problem) is #424372: :: in .info files causes Fatal error.

Committed to 7.x - thanks!

  • David_Rothstein committed ff246eb on 7.x
    Issue #2146643 by quicksketch: Speed up drupal_parse_info_format() 3x...
joseph.olstad’s picture

I'm trying to see if there's opportunities for improvement in D8 but it looks like it's already been done in D8.

In D8 I found the class "InfoParser"

This class extends the InfoParserDynamic class however it looks like the improvements have already been done in D8 in this case.

I was curious and dug through the code a bit. My time would probably be better used with XHProf for identifying opportunities for improvement.

Good work.

It would be nice now that we have jenkins that if we could also trigger XHProf tests on new code changes to see what the performance impact is during simpletest runs. Sure we've got simpletest /jenkins for assertion tests but not for latency tests. Not sure where I'd file that request. This would really help the community crack down hard on bottlenecks and give automated and relative results for all to see. XHProf was optimised by Facebook, it works great and I can definately see how it could be set up similar to Jenkins or in concert with Jenkins/Simpletest.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

RajabNatshah’s picture

Updated number of websites .. and testing change of performance