Hi Andy,
I've noticed that checkpatch.pl is getting slower and slower when run on
a whole file, but yesterday I realized that it now is pretty much
unusable:
$ time ./scripts/checkpatch.pl --file drivers/staging/uc2322/aten2011.c
<snip>
total: 168 errors, 126 warnings, 3939 lines checked
drivers/staging/uc2322/aten2011.c has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
real 8m7.924s
user 8m7.058s
sys 0m0.116s
I've applied about 10 cleanup patches to this file now, which caused it
to suddenly drop back down to a reasonable time (now around 5 seconds)
to run the script, but I can't seem to figure out what I did to make it
better (bisecting gave inconsistant results).
As the tool is something I point companies at all the time to give them
hints on what is needed to clean up their code, and is what I use daily
when cleaning up drivers/staging/ code, is there any way this can be
fixed?
I've attached the aten2011.c file below that takes so long, so that you
can try this out yourself. If you want the "faster" version of the
file, look in the next -next release, or I can send it to you as well.
thanks,
greg k-h
On Sat, Jan 31, 2009 at 10:55:07AM -0800, Greg KH wrote:
> Hi Andy,
>
> I've noticed that checkpatch.pl is getting slower and slower when run on
> a whole file, but yesterday I realized that it now is pretty much
> unusable:
>
> $ time ./scripts/checkpatch.pl --file drivers/staging/uc2322/aten2011.c
>
> <snip>
> total: 168 errors, 126 warnings, 3939 lines checked
>
> drivers/staging/uc2322/aten2011.c has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> real 8m7.924s
> user 8m7.058s
> sys 0m0.116s
That is scarey indeed. Something is very wrong in there if it went back
to a more reasonable 10's of seconds with a few patches. I will have a
look at the file you attached and see what I can find.
Thanks for the heads up.
-apw
On Sat, 2009-01-31 at 21:02 +0000, Andy Whitcroft wrote:
> On Sat, Jan 31, 2009 at 10:55:07AM -0800, Greg KH wrote:
> > Hi Andy,
> >
> > I've noticed that checkpatch.pl is getting slower and slower when run on
> > a whole file, but yesterday I realized that it now is pretty much
> > unusable:
> >
> > $ time ./scripts/checkpatch.pl --file drivers/staging/uc2322/aten2011.c
> >
> > <snip>
> > total: 168 errors, 126 warnings, 3939 lines checked
> >
> > drivers/staging/uc2322/aten2011.c has style problems, please review. If any of these errors
> > are false positives report them to the maintainer, see
> > CHECKPATCH in MAINTAINERS.
> >
> > real 8m7.924s
> > user 8m7.058s
> > sys 0m0.116s
>
> That is scarey indeed. Something is very wrong in there if it went back
> to a more reasonable 10's of seconds with a few patches. I will have a
> look at the file you attached and see what I can find.
>
> Thanks for the heads up.
The way your doing updates makes bisecting this code way too difficult..
I bisected this down to the commit for version 0.15 , but if you had all
the 0.15 patches as discrete git commits I could have actually found the
specific patch causing the problem ..
Could you start doing one fix or change per git commit?
Daniel
On Sat, 2009-01-31 at 13:35 -0800, Daniel Walker wrote:
>
> The way your doing updates makes bisecting this code way too difficult..
> I bisected this down to the commit for version 0.15 , but if you had all
> the 0.15 patches as discrete git commits I could have actually found the
> specific patch causing the problem ..
>
> Could you start doing one fix or change per git commit?
My mistake , it looks like you are now .. 0.15 was a while ago I guess..
It looks like 0.15 isn't the problem after all, it only makes it slower
not extremely slow..
Daniel
On Sat, 2009-01-31 at 21:02 +0000, Andy Whitcroft wrote:
> On Sat, Jan 31, 2009 at 10:55:07AM -0800, Greg KH wrote:
> > Hi Andy,
> >
> > I've noticed that checkpatch.pl is getting slower and slower when run on
> > a whole file, but yesterday I realized that it now is pretty much
> > unusable:
> >
> > $ time ./scripts/checkpatch.pl --file drivers/staging/uc2322/aten2011.c
> >
> > <snip>
> > total: 168 errors, 126 warnings, 3939 lines checked
> >
> > drivers/staging/uc2322/aten2011.c has style problems, please review. If any of these errors
> > are false positives report them to the maintainer, see
> > CHECKPATCH in MAINTAINERS.
> >
> > real 8m7.924s
> > user 8m7.058s
> > sys 0m0.116s
>
> That is scarey indeed. Something is very wrong in there if it went back
> to a more reasonable 10's of seconds with a few patches. I will have a
> look at the file you attached and see what I can find.
>
> Thanks for the heads up.
After some debugging it looks like it takes a long time to processes
lines similar to,
/*************************************
* Bit definitions for each register *
*************************************/
#define LCR_BITS_5 0x00 /* 5 bits/char */
#define LCR_BITS_6 0x01 /* 6 bits/char */
#define LCR_BITS_7 0x02 /* 7 bits/char */
#define LCR_BITS_8 0x03 /* 8 bits/char */
#define LCR_BITS_MASK 0x03 /* Mask for bits/char field */
There's a section in the code which has several of these. The processing
slows down when this expression matches repeatedly,
# Check for potential 'bare' types
my ($stat, $cond, $line_nr_next, $remain_next, $off_next);
if ($realcnt && $line =~ /.\s*\S/) {
($stat, $cond, $line_nr_next, $remain_next, $off_next) =
ctx_statement_block($linenr, $realcnt, 0);
and ctx_statement_block will process $realcnt lines trying to find a
complete block, and it has no concept of "define" so it just keep
processing until it's concept of a block ending.. That happens on each
"define" line in the file, which I think accounts for all the overhead.
I added the following temporary work around, which speeds things up
considerably. Just forcing it to only process one line at most.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 45eb0ae..787423a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1357,7 +1357,7 @@ sub process {
my ($stat, $cond, $line_nr_next, $remain_next, $off_next);
if ($realcnt && $line =~ /.\s*\S/) {
($stat, $cond, $line_nr_next, $remain_next, $off_next) =
- ctx_statement_block($linenr, $realcnt, 0);
+ ctx_statement_block($linenr, 1, 0);
$stat =~ s/\n./\n /g;
$cond =~ s/\n./\n /g;
On Sat, Jan 31, 2009 at 04:57:57PM -0800, Daniel Walker wrote:
> On Sat, 2009-01-31 at 21:02 +0000, Andy Whitcroft wrote:
> > On Sat, Jan 31, 2009 at 10:55:07AM -0800, Greg KH wrote:
> > > Hi Andy,
> > >
> > > I've noticed that checkpatch.pl is getting slower and slower when run on
> > > a whole file, but yesterday I realized that it now is pretty much
> > > unusable:
> > >
> > > $ time ./scripts/checkpatch.pl --file drivers/staging/uc2322/aten2011.c
> > >
> > > <snip>
> > > total: 168 errors, 126 warnings, 3939 lines checked
> > >
> > > drivers/staging/uc2322/aten2011.c has style problems, please review. If any of these errors
> > > are false positives report them to the maintainer, see
> > > CHECKPATCH in MAINTAINERS.
> > >
> > > real 8m7.924s
> > > user 8m7.058s
> > > sys 0m0.116s
> >
> > That is scarey indeed. Something is very wrong in there if it went back
> > to a more reasonable 10's of seconds with a few patches. I will have a
> > look at the file you attached and see what I can find.
> >
> > Thanks for the heads up.
>
> After some debugging it looks like it takes a long time to processes
> lines similar to,
>
> /*************************************
> * Bit definitions for each register *
> *************************************/
> #define LCR_BITS_5 0x00 /* 5 bits/char */
> #define LCR_BITS_6 0x01 /* 6 bits/char */
> #define LCR_BITS_7 0x02 /* 7 bits/char */
> #define LCR_BITS_8 0x03 /* 8 bits/char */
> #define LCR_BITS_MASK 0x03 /* Mask for bits/char field */
>
>
> There's a section in the code which has several of these. The processing
> slows down when this expression matches repeatedly,
Ah, that makes some sense, as I did remove a lot of these that were not
being used in other patches to the file.
> # Check for potential 'bare' types
> my ($stat, $cond, $line_nr_next, $remain_next, $off_next);
> if ($realcnt && $line =~ /.\s*\S/) {
> ($stat, $cond, $line_nr_next, $remain_next, $off_next) =
> ctx_statement_block($linenr, $realcnt, 0);
>
> and ctx_statement_block will process $realcnt lines trying to find a
> complete block, and it has no concept of "define" so it just keep
> processing until it's concept of a block ending.. That happens on each
> "define" line in the file, which I think accounts for all the overhead.
>
> I added the following temporary work around, which speeds things up
> considerably. Just forcing it to only process one line at most.
But defines do cross a line, and we should be able to check them
properly, isn't that what the original version of this was doing?
thanks,
greg k-h