1. Install Drupal 6.12
  2. Enable OpenID
  3. Enter an OpenID in the user profile

Expected behaviour: being redirected to OpenID provider

Actual result: Loads empty page after half a minute

Apache error log:
PHP Fatal error: Maximum execution time of 30 seconds exceeded in /drupal/modules/openid/openid.module on line 363

This line calls the bcpowmod function.

I simultaneously had this exact same problem on an installation of the Serendipity blog software, which also failed at that particular function during OpenID setup. However in that case the problem went away as I installed the gmp package for my PHP5 distribution, as the seredipity module seem to give gmp math functions precedence.

NB. my server is kind of old, but I don't think it should time out. After installing gmp, Serendipity execution is almost instant.

BCMath is compiled in. Assuming BCmath is that much slower than gmp then one possible solution would be adding support for gmp as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous’s picture

Version: 6.12 » 6.14

Actually this extreme execution time seems to only be the case on the very addition of or first time login with a new OpenID. On subsequent logins it's way faster. So I suspect that some initial heavy lifting is being done the first time.

c960657’s picture

Title: Maximum execution time in openid module » Make OpenID module use GMP math library instead of BCMath
Version: 6.14 » 7.x-dev
Issue tags: +Performance

Agreed, we should use GMP if possible. I got similar results when comparing GMP and BCMath years ago.

I wonder whether we can safely assume that people have GMP installed, or whether we have to add wrapper functions with support for both libraries?

walkah’s picture

Assigned: Unassigned » walkah

You can not assume that gmp will be available. I had a patch that I rolled for an old client that used GMP (if available) and fell back to BCMath. I will revisit this patch and re-roll for D7.

walkah’s picture

Status: Active » Needs review
FileSize
7.74 KB

Attached is a patch that adds math wrapper functions - giving preference to GMP (the faster option) if available, and falling back to BCMath. It also includes a hook_requirements() update that will suggest (REQUIREMENTS_WARNING) installing GMP if only BCMath is available.

walkah’s picture

FileSize
7.95 KB

Oh for the love. I guess it's been a while :-P

(diff'ed from the proper directory this time)

Dries’s picture

Version: 7.x-dev » 8.x-dev

I'm going to push this out to Drupal 8. If you think this is important for Drupal 7, please provide benchmarks to make the case for this.

spiderman’s picture

I've re-rolled walkah's patch from #7 against the 6.19 release of the core OpenID module, although I haven't been able to absolutely confirm that it's improved this issue on my installation. Testing and feedback from others welcome :)

Anonymous’s picture

The OP "benchmark" was that this times out after 30 secs on a 450 MHz i386 core doing nothing else. A somewhat more common situation would be a VPS or a cloud instance with about twice that performance. That would normally handle a fair deal of concurrent traffic (moving the bottleneck to some other subsystem). However, it would probably not handle more than two concurrent OpenID signups, taking tens of seconds each, let alone handle the regular browsing.

This misuse of BCMath could easily lead to a lot of potential customers getting a poor first impression or even no impression at all: OpenID account setup is a situation that is not unlikely to peak with lots of concurrence in the event of, let's say, a marketing campaign or upon being "slashdotted".

Anyway, the reason for this entry is a security question: If so, wouldn't this also be a handy DOS attack vector if registration is unprotected (which it is by default)?

jbrown’s picture

The wrapper functions should be part of the Drupal API, not just in the openid module.

catch’s picture

Version: 8.x-dev » 7.x-dev
Priority: Normal » Major
FileSize
132.46 KB

No benchmarks, but here's a screenshot from webgrind. The total request takes 8 seconds, two calls to bcpowmod (to a server not doing a lot else at the time except profiling the request) took 4 seconds between them. I'm moving this back to D7 since there's no API change here, and four seconds (plus whatever else the request does) is really bad both from a user experience point of view, and server load.

Status: Needs review » Needs work

The last submitted patch, openid.gmp_.drupal-org-488062.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
7.95 KB

Re-upping walkah's patch from #7.

Gábor Hojtsy’s picture

Woah, that sounds bad. I'd even support this getting into Drupal 6 if that means this much improvement (and I'm seeing no API change :). However, a possibly not too minor nitpick is that I'd not name the requirements item simply "math", that has a good chance of collision with contribs. Maybe name it "openid_math"? Now that its exact library is not anymore possible to tell beforehand. That is not a big change, so not moving off of needs review.

paul.lovvik’s picture

FileSize
8.33 KB

Rerolled the patch with the 'math' -> 'openid_math' change.

c960657’s picture

This is a nice improvement.

hook_requirements() complains if BCMath is not available, but this doesn't matter, because we can just use GMP instead. It seems sufficient that just one of the two libraries are available. Actually, AFAIK OpenID will work even if none of the libraries are available (though this isn't even reflected in the current hook_requirements() message) — it will then just run without associations, and that should be just as secure (I think) but slower (because it needs to do an extra server-to-server HTTP request every time a user logs in).

The comment // If bcmath is present, then create an association should be updated.

bcdiv() is only called from _openid_dh_long_to_binary(). With GMP, _openid_dh_long_to_binary() could be implemented in just a few lines of code (by converting to hexadecimal and then translating that to binary using pack('H*')). So as an alternative to implementing _openid_math_div(), we could implement _openid_math_long_to_binary() instead. That would make the two code-paths much more different with questionable benefit, so I personally wouldn't recommend it. I just wanted to mention the possibility.

I suggest renaming _openid_math_comp() to _openid_math_cmp() for consistency with "cmp" used in the function names of strcmp() and various other string comparison functions. GMP also uses gmp_cmp(), so BCMath seems like the odd man out here.

+    } elseif (function_exists('bcadd')) {

A newline should be inserted before elseif. This is relevant for the hook_requirements() change also.

The patch does change the API slightly. If GMP is enabled, _openid_dh_binary_to_long() no longer returns a string of decimal digits but a GMP resource. This can be fixed by calling gmp_strval() an appropriate place.

catch’s picture

Tried to deal with c960657s points:

hook_requirements() now emits only a warning if you don't have bcmath or gmpmath installed, and recommends gmpmath (both if neither are installed, or if only bc is).

Updated the comment that only mentioned bcmath.

_openid_math_comp() to _openid_math_cmp() - done.

elseif newline added.

wrapped gmp_add() in gmp_strval() to remove the API change.

Confirmed with profiling that the gmp library has almost no overhead at all - it replaces ~4000ms from one bc function (called twice) to around 11ms, so this is a massive improvement.

Gábor Hojtsy’s picture

Title: Make OpenID module use GMP math library instead of BCMath » Speed up OpenID by using GMP instead of BCMath if available

Let's retitle to better explain what is happening here. Patch looks good on visual review.

paul.lovvik’s picture

Status: Needs review » Reviewed & tested by the community

I have reviewed the patch and have tested it. It is working well and I was able to record a 2 second improvement with this patch (timing screen to screen responsiveness from the user's perspective).

klausi’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.63 KB

Fixed all that trailing white spaces.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Don't think we nee retesting for whitespace changes only. Testbot still happy.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to be a pain, but we need PHPDoc on those new functions. It's not clear at all to the passerby without issue context as to why we have those wrapper functions there.

I'm fine getting this in otherwise since Gábor seems keen.

catch’s picture

Status: Needs work » Needs review
FileSize
9.31 KB

Added some boilerplate PHP doc to each function, also added a bit more explanation to _openid_get_math_library().

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

#18 works for us in production. Since the only difference with #24 is doc/whitespace I think we can call this RTBC again.

walkah’s picture

+1 to committing #24 - sad this didn't make into 7.0 - but it's worth adding for performance.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Can it go in 6.x too (and 5.x contrib?)

David_Rothstein’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs review
FileSize
1.19 KB

The above patch introduced a grammatical error. Since it broke string freeze already, let's fix that quickly so it doesn't break strings anymore.

catch’s picture

Status: Needs review » Needs work

OpenID is singular, so "OpenID suggests" seems right to me?

Either way though, the the is in both HEAD and the patched version.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

Wow, I have no idea how I saw the one thing but missed the double "the"...

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

ok, let's get it in D6 too.

andypost’s picture

Status: Patch (to be ported) » Needs review
FileSize
8.46 KB

Re-roll against D6, patch also adds openid_requirements() implementation - probably Gabor could remove this patch because it adds some new strings for translation of D6 core

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The new strings are not a biggie I think, we do not change existing ones. However, looks like this will suddenly make a warning appear for people running OpenID on environments that were previously deemed entirely fine. Not sure we want to make that happen... Maybe for Drupal 6, make that an OK state as well, but print a suggestion that you can speed it up further? I'm reluctant to raise warning flags on perfectly running sites.

andypost’s picture

@ Gábor Hojtsy Maybe just throw away hook_requiremnts()?

Suppose having no BC math installed should lead to WSD but most of users that have openid running should already have this php-module

Suppose we should change this hook to check for both libraries into:
1) ($phase == 'install') - check for libraries, this gives ability to stop installing openid-module
2) runtime should be changed to REQUIREMENT_INFO about optimization

latter I'll try to re-roll a patch, suppose we should add this to D7 too

Glad to hear any responces?

nagba’s picture

Status: Needs work » Needs review
FileSize
7.94 KB

I have rerolled the #35 patch so it may apply cleanly with git. I have also changed the hook_requirement severities to REQUIREMENT_INFO.

nagba’s picture

now without drupal_static, since that's a D7 thing

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.