1116118 - 003safesys.t shouldn't compile all files by default

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Gervase Markham [:gerv]]

003safesys.t takes 88 seconds to run on BMO codebase. Much of this time is scanning all of the extensions. I _think_ it's because, for every file, it loads an entirely new copy of Bugzilla plus all the extensions into memory, which is nuts just to check if we are using system() and exec() correctly.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This pretty naive approach of using a pre-grep for the calls in question cuts it down to 5 seconds.

(Note: this isn't a problem in stock Bugzilla, where the test takes 1.3 seconds even without the patch. I think there's an N*M thing here - the more extensions, the longer it takes to load, and it has to load it more times.)

Gerv

Comment 2

[:glob ✱]

Comment on attachment 8542138 [details] [diff] [review]
Patch v.1

Review of attachment 8542138 [details] [diff] [review]:
-----------------------------------------------------------------

nice catch.

::: t/003safesys.t
@@ +46,5 @@
>      }
>  }
>  
>  my @testitems = @Support::Files::testitems; 
> +@testitems = grep { `grep -E "(system|exec)\\(" $_` } @testitems;

use perl instead of spawning grep (use File::Slurp to read the file quickly).  `grep` isn't windows friendly, and perl's pretty good at this sort of stuff ;)

i would also prefer to see this test moved inside the foreach loop with the file marked as ok instead of not including the file for tesing.

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Down to 2 wallclock seconds :-)

Gerv

Comment 4

[Frédéric Buclin]

Just to be sure: how long does it take to run 001compile.t? Because 001 and 003 have the same code, so it doesn't make sense for 003 to be much slower than 001.

Comment 5

[Frédéric Buclin]

Comment on attachment 8569186 [details] [diff] [review]
Patch v.2

>+    if ($contents !~ /(system|exec)\(/) {

Err wait. You are looking for system( and exec(, but there are some system calls with no parens, i.e. "system $foo" instead of system($foo), for instance in Bugzilla/JobQueue.pm. You should also look for words instead of substrings, using \b: \b(system|exec)\b.

Comment 6

[:glob ✱]

(In reply to Frédéric Buclin from comment #4)
> Just to be sure: how long does it take to run 001compile.t?

on my system 001 takes ~35s, 003 takes ~1m40s (unpatched) or ~10s (patched, using \b(system|exec)\b)

> Because 001 and 003 have the same code, so it doesn't make sense for 003
> to be much slower than 001.

001 doesn't test extensions, 003 does.

Comment 7

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #6)
> on my system 001 takes ~35s, 003 takes ~1m40s (unpatched)

That's really weird, because my 001compile.t takes ~40s, and 003safesys.t only ~2s.


> 001 doesn't test extensions, 003 does.

Not true. They test exactly the same files + 001 test some additional .t files.

Comment 8

[Frédéric Buclin]

The big difference between 001 and 003, despite they test the same files, is that 001 uses use_ok() to test .pm files while 003 invokes a perl interpreter in all cases, which is *much* slower.

Now, this doesn't explain why 003 is so much faster in my case compared to 001. :)

Comment 9

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #8)
> Now, this doesn't explain why 003 is so much faster in my case compared to
> 001. :)

Ah, I found why, see bug 1137669.

Comment 10

[Gervase Markham [:gerv]]

glob: are you still going to check in a (modified, using \b) patch to fix this in the BMO 4.2 codebase?

Gerv

Comment 11

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #10)
> glob: are you still going to check in a (modified, using \b) patch to fix
> this in the BMO 4.2 codebase?

yes, once there's an r+'d patch here i'll backport it to bmo.

Comment 12

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #11) 
> yes, once there's an r+'d patch here i'll backport it to bmo.

Bug 1137674 means it doesn't need fixing on trunk, AIUI. It's only BMO which needs this fix.

Gerv

Comment 13

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #12)
> (In reply to Byron Jones ‹:glob› from comment #11) 
> > yes, once there's an r+'d patch here i'll backport it to bmo.
> 
> Bug 1137674 means it doesn't need fixing on trunk, AIUI. It's only BMO which
> needs this fix.

it's still a bug on 5.0, but let's make this bmo-only.

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.3

The \b\b version is 2.5x slower, but it's still only 8 seconds, so whatever. :-)

Gerv

Comment 15

[:glob ✱]

Comment on attachment 8604191 [details] [diff] [review]
Patch v.3

Review of attachment 8604191 [details] [diff] [review]:
-----------------------------------------------------------------

great :) r=glob

Comment 16

[Gervase Markham [:gerv]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   0215f11..53af86d  master -> master

Gerv