Closed Bug 1116118 Opened 6 years ago Closed 6 years ago

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

Categories

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

Production
x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gerv, Assigned: gerv)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch Patch v.1 (obsolete) — Splinter Review
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
Assignee: nobody → gerv
Status: NEW → ASSIGNED
Attachment #8542138 - Flags: review?(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.
Attachment #8542138 - Flags: review?(glob) → review-
Component: General → Testing Suite
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: default-qa
Version: Production → unspecified
Attached patch Patch v.2 (obsolete) — Splinter Review
Down to 2 wallclock seconds :-)

Gerv
Attachment #8542138 - Attachment is obsolete: true
Attachment #8569186 - Flags: review?(glob)
Summary: 003safesys.t takes 88 seconds to run on BMO codebase → 003safesys.t shouldn't compile all files by default
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 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.
Attachment #8569186 - Flags: review?(glob) → review-
(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.
(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.
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. :)
Depends on: 1137669
(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.
See Also: → 1137674
glob: are you still going to check in a (modified, using \b) patch to fix this in the BMO 4.2 codebase?

Gerv
Flags: needinfo?(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.
Flags: needinfo?(glob)
(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
Flags: needinfo?(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.
Component: Testing Suite → General
Flags: needinfo?(glob)
Product: Bugzilla → bugzilla.mozilla.org
QA Contact: default-qa
Version: unspecified → Production
Attached patch Patch v.3Splinter Review
The \b\b version is 2.5x slower, but it's still only 8 seconds, so whatever. :-)

Gerv
Attachment #8569186 - Attachment is obsolete: true
Attachment #8604191 - Flags: review?(glob)
Comment on attachment 8604191 [details] [diff] [review]
Patch v.3

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

great :) r=glob
Attachment #8604191 - Flags: review?(glob) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   0215f11..53af86d  master -> master

Gerv
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.