Bug 74940 – [Coverity] Add initializers in YarrPattern

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Greg Billock]

[Coverity] Add initializers in YarrPattern

[Greg Billock]

Created attachment 120040 [details]
Patch

[Greg Billock]

Created attachment 120061 [details]
Patch

[Greg Billock]

Created attachment 120075 [details]
Patch

[Alexey Proskuryakov]

How does this affect performance?

[Greg Billock]

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.

[Peter Varga]

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.

[Greg Billock]

Thanks, Peter.

[Greg Billock]

Any further concerns?

[Alexis Menard (darktears)]

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.

[Greg Billock]

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.

[Greg Billock]

Created attachment 120980 [details]
Patch

[WebKit Review Bot]

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.

[Greg Billock]

Created attachment 120981 [details]
Patch

[WebKit Review Bot]

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.

[Greg Billock]

Were these errors caused by me getting a bad sync? Should I resync and re-upload?

[Greg Billock]

Created attachment 121176 [details]
Patch

[Filip Pizlo]

(In reply to comment #16)
> Created an attachment (id=121176) [details]
> Patch

What is the performance effect, if any, of these changes?

[Greg Billock]

(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.

[Filip Pizlo]

(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.

[Greg Billock]

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.

[Hajime Morrita]

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.

[Greg Billock]

I keep thinking I'll work on this and keep not. Going to close.