Get rid of autoload #162
Comments
|
This is already on my (mental) TODO list, and yes it needs to happen; more so because i am interested in the coverage metrics for the tests rather than the compile time optimisation. |
|
Yes I don't claim it would make it any faster; there isn't really any compile-time optimization between different subroutines and they are compiled to basically the same bytecode if you autoload them at run time. I'm just saying it would be simpler code and not noticeably slower. If you just rip out the autoload lines and make them ordinary subroutine definitions, does anything break? It could be quite a painless change. |
|
CGI.pm is "only" about 4000 lines of code +4000 lines of POD, which isn't really that big compared to some stuff on CPAN; so i agree that the string evaling and AUTOLOAD stuff isn't as much of an optimisation as it once was. Arguably the overhead of calling AUTOLOAD and such might now be less optimal for persistent processes. So i agree we should rip it all out and make it function sans AUTOLOAD. It will be cleaner, quicker in normal use (probably, benchmark maybe but i'm not that interested in benchmarks for this), make test coverage metrics accurate, and will finally syntax highlight correctly in vim :) |
but there's a problem - html tag functions are auto generated so we might be missing a few here. needs more tests, not confident we can just purge all this code without introducing bugs and/or breaking back compatibility for some users. also the t/pretty.t test is now failing (TODO: investigate) N.B. should this branch ever be merged it should be squashed due to the failing test in this commit
|
OK, i've had a quick look to remind myself why i hadn't already done this:
I might have another look at this over the xmas holidays. For the meantime i've had a quick crack at this and what i've done can be seen in 03de44b |
|
Digging deeper there is some AUTOLOAD magic going on in CGI::Pretty and my above changes break those. I am going to deprecate the CGI::Pretty module to solve this problem. Yep. |
to keep back compatibility we have to auto-generate all the html tag functions. this change also deprecated CGI::Pretty as it was doing magic with its own AUTOLOAD function and i'm not interested in the continued maintenance of this module, alternatives are suggested in the POD split out CGI.pm POD into CGI.pod to cut the module size in half so it's easier to work with this changes needs significant testing, since the test coverage for the html tag functions isn't comprehensive (we can delete lots of the tags from the _all_html_tags method with no test failures) some users of CGI.pm maybe relying (without realising) on AUTOLOAD functionality catching bugs in their code, or they may have code that relies on AUTOLOAD in other ways - if they are then this change will break their code.
|
Done! But it needs testing as i have the feeling this may break back compatibility for some users (largely down to the complete removal of AUTOLOAD) |
|
You'd have to go through a deprecation cycle for CGI::Pretty and then remove CGI::Pretty before releasing this change, no? |
|
Perhaps, but this change marks CGI::Pretty as deprecated; it continues to function insomuch as not breaking any code using it, the functions will just defer to CGI so not actually make the passed HTML "pretty". |
|
Doesn't look like there's much using CGI::Pretty on CPAN: http://grep.cpan.me/?q=CGI%3A%3APretty So i can manually raise tickets against those dists to let the authors downstream know that it will be deprecated soon. DarkPAN i'm not so interested in, but the deprecation will be soft (in the aforementioned sense that it will warn and defer to CGI). |
for now we just warn on import, eventually this module is going to be removed (or just be an empty shell that will use parent CGI to get at the html functions for back compat). bump version and Changes
|
v4.12 has been sent to CPAN. This marks CGI::Pretty as deprecated. I will notify dist authors on CPAN that depend on this module over the next week or two. |
|
Here's a vote against. Actually, most of people who are using CGI.pm are using it in old-fashioned cgi scripts. If people may use a persistent environment, then it's likely that they are already running something with Plack, and using a web framework, so aren't CGI.pm users anyway. |
|
I'm using it in old-fashioned CGI scripts but I recognize that on modern hardware the messing around with autoload makes very little difference to performance. In a quick benchmark I found that the un-autoload version was slightly faster, although this may not be a statistically significant result. In any case, the time taken to compile the code in CGI.pm (which is all that autoload saves you) is dwarfed by all the other startup overheads. |
|
Can you publish your benchmarks? |
|
OK, it does look like there is about 0.03 seconds of extra startup overhead for a minimal script which loads strictures etc. then CGI. For me that is dwarfed by the 1.5s of startup overhead my script has loading all the other things it needs - but it is conceivable somebody running a highly tuned and optimized CGI script would see the difference. But surely any such person would be using mod_perl or similar? It's crazy to both care strongly about process startup performance and at the same time fork a new Perl process for each request. |
|
Why it's crazy? Currently one does not have to care and can write fast web applications with cgi scripts. Please note, not everybody is able to run their stuff on a dedicated machine. Many people have only cheap webhosting where nice stuff like fast cgi, mod_perl or plack-enabled servers are simply not available. |
|
My point is that the startup time saved by autoload is still dwarfed by the other startup time taken for any non-trivial application. The benchmark I ran didn't open any files or database connections, for example. It will make no appreciable difference in practice. |
|
Here's a quick and dirty benchmark on my system. I stuck Moose in there at the end for good measure and comparison. It seems the overhead of not having AUTOLOAD on a reasonably recent perl and machine is max about 0.03s. I think we can live with that... |
|
The only benchmark I am interested is: if you rip out autoloading, how much slower will it be? Hard numbers, not assumptions. That was what I originally asked for. |
|
Slaven R. please see my earlier msg where I gave a figure of 0.03 seconds, or the later benchmark showing 0.02 seconds. This email has been scanned by the Symantec Email Security.cloud service. |
|
@eserte - 0.03s overhead with no AUTOLOAD on my machine. See the above figures. |
|
It looks worse on my system (debian/wheezy) (pistachio-perl = perl 5.20.1) The gh/162_remove_autoload branch: master: And unfortunately master is already much slower than older CGI.pm's. Here's the system perl with system CGI.pm (3.52) for comparison: (Running the system perl against master shows about the same result as with 5.20.1, so it's not the self-compiled perl being slow). This is not quite a significant slowdown. What about making CGI.pm again as fast as it used to be? |
Oh please. You think a difference of less than 0.05s is worth worrying about? I'm sorry, but unless you're running tight algorithms in heavily optimised time critical code, i can't help but think you're trolling at this point. And if you are running that kind of code you certainly wouldn't be using none-persistent CGI processes. |
|
And the next step will be the inclusion of Moose into CGI.pm... I think I'll stop this "discussion" and prepare myself for migrating away from CGI.pm. |
4.13 2014-12-18
[ RELEASE NOTES ]
- CGI::Pretty is now DEPRECATED and will be removed in a future release.
Please see GH #162 (leejo/CGI.pm#162) for more
information and discussion (also GH #140 for HTML function deprecation
discussion: leejo/CGI.pm#140)
[ TESTING ]
- fix t\rt-84767.t for failures on Win32 platforms related to file paths
4.13 2014-12-18
[ RELEASE NOTES ]
- CGI::Pretty is now DEPRECATED and will be removed in a future release.
Please see GH #162 (leejo/CGI.pm#162) for more
information and discussion (also GH #140 for HTML function deprecation
discussion: leejo/CGI.pm#140)
[ TESTING ]
- fix t\rt-84767.t for failures on Win32 platforms related to file paths
in removing AUTOLOAD we need to check that the necessary back compat methods are still available to ensure no (or minimal) breakage. to do this t/html_functions.t has been added that both uses the -compile pragma and ->compile function and also checks a function (a()) is imported into the test's namespace. the test was run against the version of the module prior to the removal of AUTOLOAD and then the unnecessary functions were removed from the _all_html_tags() function in CGI.pm bump VERSION to dev release, document Changes
|
DEV version 4.13_01 going out to CPAN with these changes. |
|
There are quite a few tags missing in this release. I think "html_functions.t" is not really useful for testing backwards-compatibility since it only checks for methods that are defined in the code. |
|
The "html_functions.t" test is still useful for explicitly testing the output generated by _tag_func so I left it alone and created a new test to check for undefined exported functions. |
Yeah. This was more of a regression test, since i wrote it to pass pre-merge of the AUTOLOAD changes. Seems i missed a few of the weirder (probably mistakenly named) tags with different cases (Area, Tr, etc). Anyway, will get this fixed. Thanks! |
|
Another issue to consider: looks like we'll loose the '-any' pragma. At the very least, POD will need to be updated. |
|
Good catch. The -any pragma looks like a bad/dangerous thing - i've checked a few code search engines and can't see anything using it (including on CPAN or github) so i'm going to remove it and include a warning if it is used. |
there are some tags with none standard names (casing so possibly
incorrect) that we need to generate in the _all_html_tags back
compat function.
also remove and document removal of the -any pragma since this is
a bad idea (bugs waiting to happen). searching code on CPAN and
github shows nothing using it so this is going to be on the very
edge case of usage
update POD to remove references to auto generated functions and
the various no longer supported pragmas
Squashed commit of the following:
commit 7d362b2bf8b725c014f3df0922c772e4a9cfbed8
Author: Lee Johnson <lee@givengain.ch>
Date: Fri Feb 13 08:56:16 2015 +0100
Changes and VERSION bump for CPAN DEV release
commit 4e8b2d6fe59fd4673b7251935313aa5e736c19ad
Author: Lee Johnson <lee@givengain.ch>
Date: Fri Feb 13 08:48:02 2015 +0100
increase test coverage and cleanup POD
with regards to the removal of AUTOLOAD we lose the -any pragma and
the *foo pragma
commit 477c41d
Author: Stuart A Johnston <saj_git@thecommune.net>
Date: Thu Feb 12 16:56:58 2015 -0500
Need to skip start/end_html tests since they are custom
commit 8a7715e
Author: Stuart A Johnston <saj_git@thecommune.net>
Date: Thu Feb 12 16:37:03 2015 -0500
Add missing html funcs
commit 9733f89
Author: Stuart A Johnston <saj_git@thecommune.net>
Date: Thu Feb 12 16:35:46 2015 -0500
Check for specific error - save_parameters dies without a param
commit 20f4076
Author: Stuart A Johnston <saj_git@thecommune.net>
Date: Thu Feb 12 14:16:31 2015 -0500
Added test to check that all exported HTML tag functions are defined
|
I do suggest a deprecation cycle will be needed for these things. CGI.pm has a lot of quite ancient code using it, which won't show up on CPAN or in code search engines - perhaps more for CGI.pm than for any other module. At the least, it needs to wait until after CGI.pm is unbundled from the perl core, since a new perl core release has fairly high backwards-compatibility requirement. |
v3.65 was the last version of CGI.pm to go out with the perl core so the backwards-compatibility requirement is no longer a hard one (although i am making efforts to retain this in the v4 releases). You can't get a more recent version of CGI.pm unless you use CPAN, a vendor package, or install manually; if you're doing this then you should be testing any upgrade no matter how trivial (debian, et al, essentially maintain their own fork of perl modules so all bets are off if you're using a vendor-package). If an update breaks your code then you either fix your code or stick with the last version that didn't break your code. For edge case stuff like this i'm not so concerned with backwards-compatibility, and the cruft needs to go. |
|
Ah, so it has already been dropped from perl core for the purposes of future releases. And if the new version has a bumped major number to 4, that should indicate some cruft-clearing has happened ;-p |
Yep, and i'm trying to get the word out that changes are happening. Even so, there will always be users that will install CGI.pm (either directly or indirectly) and just expect it to work. No amount of noise or number of deprecation cycles is going to make them aware that their code might [soon] break, hence treading carefully with the v4 release. I'm really going to gut the module in the v5 releases, but that won't happen until next year at the earliest. |
|
In testing this release against my codebase, I've noticed a change in output for the special-case tags, Tr, TR, Sub, Area, etc. In previous versions of CGI.pm, the tags are always lowercase in the output regardless of the method name. Since HTML is not case sensitive, it might not be a problem. I just wanted to at least mention it. |
changes eval to a closure. reduces compile time from 0.04s to 0.035s (on my machine, averaged over 100 runs). as suggest at http://www.perlmonks.org/?node_id=1117952 don't warn when calling compile as this is now the default so it's not actually deprecated make _tag_func lowercase HTML tags to be back compatible (HTML isn't case sensitive for tags, but other code may have tests that rely on this behaviour)
|
v4.14 on its way to CPAN. |
|
I realize I'm quite late to the party here, but due to stable running code, I haven't had to look at this until now. While I don't often use CGI::Pretty in production, I have found that it is VERY useful for generating HTML code that can be read for debugging purposes. There are, of course, alternatives (e.g. parsers that will make convert generated HTML code to pretty so it can be read), but adding yet another dependency in the debugging chain introduces yet additional places to hunt for bugs. As such, if CGI::Pretty could be resurrected, that would be appreciated. If not, c'est la vie, but it's a painful parting. |
Back in olden days autoload might have shaved a little off CGI.pm startup time, but CPUs are faster than they used to be (relative to disk I/O and all the other things which take time), and besides, any web application where performance is important doesn't start a new perl process (and load CGI.pm afresh) for each request.
I suggest simplifying the code by removing the autoload stuff and just having ordinary subroutines parsed at compile time.
The text was updated successfully, but these errors were encountered: