If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Optimize check_spidermonkey_style.py

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8485397 [details] [diff] [review]
Patch

Since bug 1063728, check_spidermonkey_style.py runs as part of every build (instead of |make check|).

The script takes more than 3 seconds on my machine. The main problem is in do_file: we execute 5 regular expressions for each line of code. This patch makes us skip lines that do not contain '#' to avoid this for most lines.

This improves the time from 3.6 to 1.4 seconds.

Almost all of the remaining time (1.1 s) is spent running |hg manifest -q| (or a similar git command) to list all tracked files.
Attachment #8485397 - Flags: review?(n.nethercote)
(Assignee)

Comment 1

3 years ago
(In reply to Jan de Mooij [:jandem] from comment #0)
> Almost all of the remaining time (1.1 s) is spent running |hg manifest -q|
> (or a similar git command) to list all tracked files.

FWIW, the following command takes 0.4s in the shell:

  hg status -nacm -I '**.h' -I '**.cpp' -I '**.tbl' -I '**.msg' js/ mfbt/

It's not trivial to use that in get_all_filenames(), as the Python script runs in js/src and the hg command returns paths relative to that. It also seems a bit less maintainable so I'll leave this alone for now, but if somebody is interested in making the script faster that might be a good place to start.
Comment on attachment 8485397 [details] [diff] [review]
Patch

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

Thank you!

::: config/check_spidermonkey_style.py
@@ -483,5 @@
>                    include1.quote() + ' should be included after ' + include2.quote())
>  
> -    # The #include statements in the files in assembler/ have all manner of implicit
> -    # ordering requirements.  Boo.  Ignore them.
> -    skip_order_checking = inclname.startswith('assembler/')

Nice :)
Attachment #8485397 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 3

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e4fe104f04
(In reply to Jan de Mooij [:jandem] from comment #0)
> Almost all of the remaining time (1.1 s) is spent running |hg manifest -q|
> (or a similar git command) to list all tracked files.

Err, why do we do this?  Lars has been complaining about this (bug 1063728 comment 18).
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4)
> (In reply to Jan de Mooij [:jandem] from comment #0)
> > Almost all of the remaining time (1.1 s) is spent running |hg manifest -q|
> > (or a similar git command) to list all tracked files.
> 
> Err, why do we do this?  Lars has been complaining about this (bug 1063728
> comment 18).

It's a reliable way to get all the source files without other files that might be in the srcdir causing confusion. (E.g. if you put a build dir within the srcdir). If there's a better way please feel free to change it.

This script was written in a fashion that was appropriate for the way it was being invoked. As a result of this bug it's now being invoked more frequently, and in a wider range of circumstances, which is exposing some limitations. That's worth keeping in mind.
It would be nicest if the script could ask the build system directly, so the script will know about any file that's part of the build (for the subset of files it cares about).
https://hg.mozilla.org/mozilla-central/rev/f9e4fe104f04
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.