- Install Drupal 6.12
- Enable OpenID
- 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.
Comment | File | Size | Author |
---|---|---|---|
#39 | openid-performance-patch-d6-488062-39.patch | 7.91 KB | nagba |
#38 | openid-performance-patch-d6-488062-38.patch | 7.94 KB | nagba |
#35 | 488062-openid-d6.patch | 8.46 KB | andypost |
#31 | 488062-grammar-31.patch | 1.19 KB | David_Rothstein |
#29 | 488062-grammar.patch | 1.19 KB | David_Rothstein |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedOn the topic of bc slowness and purpose:
http://lists.danga.com/pipermail/yadis/2005-August/001340.html
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedActually 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.
Comment #3
c960657 CreditAttribution: c960657 commentedAgreed, 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?
Comment #4
walkah CreditAttribution: walkah commentedYou 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.
Comment #6
walkah CreditAttribution: walkah commentedAttached 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.
Comment #7
walkah CreditAttribution: walkah commentedOh for the love. I guess it's been a while :-P
(diff'ed from the proper directory this time)
Comment #8
Dries CreditAttribution: Dries commentedI'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.
Comment #9
spidermanI'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 :)
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedThe 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)?
Comment #11
jbrown CreditAttribution: jbrown commentedThe wrapper functions should be part of the Drupal API, not just in the openid module.
Comment #12
catchNo 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.
Comment #14
catchRe-upping walkah's patch from #7.
Comment #15
Gábor HojtsyWoah, 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.
Comment #16
paul.lovvik CreditAttribution: paul.lovvik commentedRerolled the patch with the 'math' -> 'openid_math' change.
Comment #17
c960657 CreditAttribution: c960657 commentedThis 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.
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.
Comment #18
catchTried 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.
Comment #19
Gábor HojtsyLet's retitle to better explain what is happening here. Patch looks good on visual review.
Comment #20
paul.lovvik CreditAttribution: paul.lovvik commentedI 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).
Comment #21
klausiFixed all that trailing white spaces.
Comment #22
Gábor HojtsyDon't think we nee retesting for whitespace changes only. Testbot still happy.
Comment #23
webchickSorry 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.
Comment #24
catchAdded some boilerplate PHP doc to each function, also added a bit more explanation to _openid_get_math_library().
Comment #25
pwolanin CreditAttribution: pwolanin commented#18 works for us in production. Since the only difference with #24 is doc/whitespace I think we can call this RTBC again.
Comment #26
walkah CreditAttribution: walkah commented+1 to committing #24 - sad this didn't make into 7.0 - but it's worth adding for performance.
Comment #27
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #28
pwolanin CreditAttribution: pwolanin commentedCan it go in 6.x too (and 5.x contrib?)
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedThe above patch introduced a grammatical error. Since it broke string freeze already, let's fix that quickly so it doesn't break strings anymore.
Comment #30
catchOpenID is singular, so "OpenID suggests" seems right to me?
Either way though,
the the
is in both HEAD and the patched version.Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedWow, I have no idea how I saw the one thing but missed the double "the"...
Comment #32
catchLooks good.
Comment #33
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #34
pwolanin CreditAttribution: pwolanin commentedok, let's get it in D6 too.
Comment #35
andypostRe-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
Comment #36
Gábor HojtsyThe 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.
Comment #37
andypost@ 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?
Comment #38
nagba CreditAttribution: nagba commentedI have rerolled the #35 patch so it may apply cleanly with git. I have also changed the hook_requirement severities to REQUIREMENT_INFO.
Comment #39
nagba CreditAttribution: nagba commentednow without drupal_static, since that's a D7 thing