Bug 74940 - [Coverity] Add initializers in YarrPattern
Summary: [Coverity] Add initializers in YarrPattern
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Greg Billock
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-20 10:39 PST by Greg Billock
Modified: 2012-06-25 09:52 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.76 KB, patch)
2011-12-20 10:40 PST, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (6.94 KB, patch)
2011-12-20 12:38 PST, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (8.78 KB, patch)
2011-12-20 14:00 PST, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (8.73 KB, patch)
2012-01-03 12:54 PST, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (8.69 KB, patch)
2012-01-03 12:59 PST, Greg Billock
no flags Details | Formatted Diff | Diff
Patch (8.72 KB, patch)
2012-01-04 15:35 PST, Greg Billock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Billock 2011-12-20 10:39:12 PST
[Coverity] Add initializers in YarrPattern
Comment 1 Greg Billock 2011-12-20 10:40:40 PST
Created attachment 120040 [details]
Patch
Comment 2 Greg Billock 2011-12-20 12:38:26 PST
Created attachment 120061 [details]
Patch
Comment 3 Greg Billock 2011-12-20 14:00:47 PST
Created attachment 120075 [details]
Patch
Comment 4 Alexey Proskuryakov 2011-12-20 16:58:05 PST
How does this affect performance?
Comment 5 Greg Billock 2011-12-20 22:02:14 PST
I haven't tested it yet, but moving some of these initializations to the initializer list ought to more than make up for a couple extra integer assignments.
Comment 6 Peter Varga 2011-12-21 02:19:00 PST
I don't think so it affects performance but this change seems to me reasonable.

Anyway I tested it for regressions (on fast/js and fast/regex) and it passed.
Comment 7 Greg Billock 2011-12-21 10:34:03 PST
Thanks, Peter.
Comment 8 Greg Billock 2011-12-22 10:45:15 PST
Any further concerns?
Comment 9 Alexis Menard (darktears) 2012-01-02 12:55:41 PST
Comment on attachment 120075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120075&action=review

LGTM.

> Source/JavaScriptCore/yarr/YarrPattern.cpp:237
> +

Unnecessary space addition.
Comment 10 Greg Billock 2012-01-03 12:53:19 PST
Comment on attachment 120075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120075&action=review

>> Source/JavaScriptCore/yarr/YarrPattern.cpp:237
>> +
> 
> Unnecessary space addition.

Fixed.
Comment 11 Greg Billock 2012-01-03 12:54:35 PST
Created attachment 120980 [details]
Patch
Comment 12 WebKit Review Bot 2012-01-03 12:56:12 PST
Attachment 120980 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 3a7674dadd24ecd6a1b023eba203ecf2ed62fc59 != edb861612940a3244431d239dacdbc36317b627c
rereading e1e2538cff7d320c2b7cc22c0b75c9369ce9cdb2
	A	LayoutTests/svg/custom/webkit-transform-crash.html
	A	LayoutTests/svg/custom/webkit-transform-crash-expected.txt
	M	LayoutTests/ChangeLog
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/svg/SVGStyledTransformableElement.cpp
103950 = 8282a1d85ad097ce4064a5896912bdb211b115e6 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 158.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Greg Billock 2012-01-03 12:59:29 PST
Created attachment 120981 [details]
Patch
Comment 14 WebKit Review Bot 2012-01-03 13:00:58 PST
Attachment 120981 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 3a7674dadd24ecd6a1b023eba203ecf2ed62fc59 != edb861612940a3244431d239dacdbc36317b627c
rereading e1e2538cff7d320c2b7cc22c0b75c9369ce9cdb2
	A	LayoutTests/svg/custom/webkit-transform-crash.html
	A	LayoutTests/svg/custom/webkit-transform-crash-expected.txt
	M	LayoutTests/ChangeLog
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/svg/SVGStyledTransformableElement.cpp
103950 = 8282a1d85ad097ce4064a5896912bdb211b115e6 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 158.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Greg Billock 2012-01-04 13:17:33 PST
Were these errors caused by me getting a bad sync? Should I resync and re-upload?
Comment 16 Greg Billock 2012-01-04 15:35:01 PST
Created attachment 121176 [details]
Patch
Comment 17 Filip Pizlo 2012-01-11 12:33:56 PST
(In reply to comment #16)
> Created an attachment (id=121176) [details]
> Patch

What is the performance effect, if any, of these changes?
Comment 18 Greg Billock 2012-01-11 13:12:40 PST
(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=121176) [details] [details]
> > Patch
> 
> What is the performance effect, if any, of these changes?

See #6 above, but are there any other specific tests that would be better? If so, I can run them.
Comment 19 Filip Pizlo 2012-01-11 13:18:16 PST
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Created an attachment (id=121176) [details] [details] [details]
> > > Patch
> > 
> > What is the performance effect, if any, of these changes?
> 
> See #6 above, but are there any other specific tests that would be better? If so, I can run them.

I'm interested in performance on SunSpider, V8, and Kraken.  What we've been doing is posting the perf results as reported by those harnesses on bugzilla.  What we expect to see, for pretty much any change that is intended to be neutral, is small variability in individual tests and slight (<0.5%) swings in averages.

Having a record of this is useful in two ways:

1) If we later find a perf regression and we suspect a bug, we can look up and see what the performance effect was by just looking at bugzilla.

2) It broadens our coverage of configurations.  It's possible that on my machine this change will be a 0.7% slow-down but on yours it will be a 0.5% speed-up.  Armed with both pieces of information we can conclude that it's likely neutral.  If we only have the one piece of information (0.7% regression on my machine) then we'd likely roll it out.

I particularly think that changes such as the ones you're making are worthwhile in general, but they are especially worthwhile if we have a strong body of evidence that the additional initializations do not cost us anything.
Comment 20 Greg Billock 2012-01-11 13:23:05 PST
Makes perfect sense. Will do.

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > Created an attachment (id=121176) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > What is the performance effect, if any, of these changes?
> > 
> > See #6 above, but are there any other specific tests that would be better? If so, I can run them.
> 
> I'm interested in performance on SunSpider, V8, and Kraken.  What we've been doing is posting the perf results as reported by those harnesses on bugzilla.  What we expect to see, for pretty much any change that is intended to be neutral, is small variability in individual tests and slight (<0.5%) swings in averages.
> 
> Having a record of this is useful in two ways:
> 
> 1) If we later find a perf regression and we suspect a bug, we can look up and see what the performance effect was by just looking at bugzilla.
> 
> 2) It broadens our coverage of configurations.  It's possible that on my machine this change will be a 0.7% slow-down but on yours it will be a 0.5% speed-up.  Armed with both pieces of information we can conclude that it's likely neutral.  If we only have the one piece of information (0.7% regression on my machine) then we'd likely roll it out.
> 
> I particularly think that changes such as the ones you're making are worthwhile in general, but they are especially worthwhile if we have a strong body of evidence that the additional initializations do not cost us anything.
Comment 21 Hajime Morrita 2012-01-16 00:23:53 PST
Comment on attachment 121176 [details]
Patch

Clearing r? for now to clear up the pending review queue. Fell free to r? again once you are back.
Comment 22 Greg Billock 2012-06-25 09:52:37 PDT
I keep thinking I'll work on this and keep not. Going to close.