Bug 15996

Summary:

Apply further sanity to match() function (for 10%!? speedup in dna-regexp?)

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

JavaScriptCore

Assignee:

Eric Seidel (no email) <eric>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.4

Bug Depends on:

Bug Blocks:

15998

Attachments:

Description	Flags
patch (nearly entirely search/replace)	sam: review+

Eric Seidel (no email)

Reported 2007-11-15 02:01:04 PST

Apply further sanity to match() function (for 10%!? speedup in dna-regexp?) This was not intended as a speedup patch. However SunSpider claims it's a 10% speedup to regexp-dna. It also claims it's a regression to some other random (non-regexp using!) tests. I blame bad codegen. Or the toothfairy. I can't decide which. TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 0.3% faster 4377.8ms +/- 0.1% 4365.4ms +/- 0.2% significant ============================================================================= 3d: 1.9% *slower* 497.6ms +/- 0.5% 507.0ms +/- 2.4% significant cube: 4.6% *slower* 166.8ms +/- 0.6% 174.4ms +/- 6.9% significant morph: ?? 160.4ms +/- 0.4% 160.8ms +/- 0.3% not conclusive: might be 0.2% *slower* raytrace: 0.8% *slower* 170.4ms +/- 0.7% 171.8ms +/- 0.3% significant access: 0.8% faster 717.4ms +/- 0.5% 711.4ms +/- 0.2% significant binary-trees: - 108.8ms +/- 1.7% 108.6ms +/- 0.6% fannkuch: 1.4% faster 347.6ms +/- 0.2% 342.8ms +/- 0.2% significant nbody: - 182.0ms +/- 0.5% 181.8ms +/- 0.3% nsieve: 1.0% faster 79.0ms +/- 0.0% 78.2ms +/- 0.7% significant bitops: ?? 621.4ms +/- 0.4% 622.8ms +/- 1.2% not conclusive: might be 0.2% *slower* 3bit-bits-in-byte: - 116.4ms +/- 1.6% 115.6ms +/- 1.0% bits-in-byte: ?? 153.4ms +/- 0.9% 155.6ms +/- 4.6% not conclusive: might be 1.4% *slower* bitwise-and: - 214.2ms +/- 0.3% 214.2ms +/- 0.3% nsieve-bits: - 137.4ms +/- 0.5% 137.4ms +/- 0.5% controlflow: - 155.8ms +/- 0.7% 155.4ms +/- 0.7% recursive: - 155.8ms +/- 0.7% 155.4ms +/- 0.7% crypto: - 345.0ms +/- 0.5% 343.6ms +/- 0.8% aes: - 97.8ms +/- 0.6% 97.6ms +/- 0.7% md5: - 124.2ms +/- 0.4% 123.6ms +/- 0.9% sha1: - 123.0ms +/- 0.7% 122.4ms +/- 2.1% date: ?? 368.0ms +/- 0.3% 369.2ms +/- 0.7% not conclusive: might be 0.3% *slower* format-tofte: 1.5% *slower* 160.6ms +/- 0.4% 163.0ms +/- 1.3% significant format-xparb: - 207.4ms +/- 0.7% 206.2ms +/- 0.3% math: 3.3% *slower* 510.8ms +/- 0.4% 527.6ms +/- 0.5% significant cordic: 7.0% *slower* 213.8ms +/- 0.5% 228.8ms +/- 0.6% significant partial-sums: - 174.2ms +/- 0.3% 173.8ms +/- 0.3% spectral-norm: 1.8% *slower* 122.8ms +/- 0.8% 125.0ms +/- 0.7% significant regexp: 10.0% faster 296.0ms +/- 0.3% 266.4ms +/- 0.4% significant dna: 10.0% faster 296.0ms +/- 0.3% 266.4ms +/- 0.4% significant string: - 865.8ms +/- 0.5% 862.0ms +/- 0.2% base64: 1.3% faster 122.6ms +/- 0.9% 121.0ms +/- 0.0% significant fasta: 0.4% *slower* 205.2ms +/- 0.3% 206.0ms +/- 0.4% significant tagcloud: - 197.0ms +/- 1.2% 196.2ms +/- 0.8% unpack-code: - 202.4ms +/- 0.5% 202.4ms +/- 0.3% validate-input: 1.6% faster 138.6ms +/- 1.2% 136.4ms +/- 1.2% significant

Attachments
patch (nearly entirely search/replace) (98.08 KB, patch) 2007-11-15 02:03 PST, Eric Seidel (no email)	sam: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2007-11-15 02:03:13 PST

Created attachment 17289 [details] patch (nearly entirely search/replace) This patch depends on bug 15995, and in fact, the SunSpider numbers above include that patch, so it's possible my other whitespace changes caused the 10% speedup... somehow I wouldn't be surprised.

Eric Seidel (no email)

Comment 2 2007-11-15 02:12:55 PST

Binary size information. Before: 1551620 After bug 15995: 1551620 After this bug: 1547524 Not sure why this change makes the binary *smaller*, but it does.

Eric Seidel (no email)

Comment 3 2007-11-15 02:14:31 PST

I guess the binary size change could be related to fewer gotos and the error reporting code no longer being inlined.

Eric Seidel (no email)

Comment 4 2007-11-15 02:19:05 PST

My latest sunspider numbers show this patch as a 0.7% speedup. Again, I don't think this is necessarily related to the patch itself, but rather some codegen side effect. Binary size being one possibility. I still think this is worth reviewing and landing. TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 0.7% faster 4379.8ms +/- 0.2% 4347.4ms +/- 0.2% significant ============================================================================= 3d: - 498.6ms +/- 0.4% 498.2ms +/- 0.4% cube: - 167.2ms +/- 0.8% 166.8ms +/- 1.1% morph: - 160.8ms +/- 0.6% 160.4ms +/- 0.4% raytrace: ?? 170.6ms +/- 0.4% 171.0ms +/- 0.0% not conclusive: might be 0.2% *slower* access: 0.7% faster 716.4ms +/- 0.2% 711.4ms +/- 0.2% significant binary-trees: 0.4% *slower* 108.0ms +/- 0.0% 108.4ms +/- 0.6% significant fannkuch: 1.5% faster 347.8ms +/- 0.2% 342.6ms +/- 0.2% significant nbody: ?? 181.8ms +/- 0.3% 182.0ms +/- 0.0% not conclusive: might be 0.1% *slower* nsieve: - 78.8ms +/- 0.7% 78.4ms +/- 0.9% bitops: 0.2% faster 621.8ms +/- 0.2% 620.4ms +/- 0.2% significant 3bit-bits-in-byte: - 115.8ms +/- 0.5% 115.4ms +/- 0.6% bits-in-byte: - 153.4ms +/- 0.4% 153.4ms +/- 0.4% bitwise-and: - 214.8ms +/- 0.3% 214.6ms +/- 0.3% nsieve-bits: 0.6% faster 137.8ms +/- 0.4% 137.0ms +/- 0.0% significant controlflow: - 156.0ms +/- 1.0% 155.6ms +/- 0.4% recursive: - 156.0ms +/- 1.0% 155.6ms +/- 0.4% crypto: 0.9% faster 344.8ms +/- 0.3% 341.8ms +/- 0.3% significant aes: - 97.6ms +/- 0.7% 97.4ms +/- 0.7% md5: 0.8% faster 124.2ms +/- 0.4% 123.2ms +/- 0.5% significant sha1: 1.5% faster 123.0ms +/- 0.7% 121.2ms +/- 1.1% significant date: - 368.8ms +/- 0.8% 367.4ms +/- 0.5% format-tofte: - 161.8ms +/- 1.5% 161.2ms +/- 0.6% format-xparb: - 207.0ms +/- 0.9% 206.2ms +/- 0.7% math: 3.2% *slower* 511.4ms +/- 0.5% 527.6ms +/- 0.8% significant cordic: 6.5% *slower* 215.0ms +/- 0.8% 229.0ms +/- 1.2% significant partial-sums: - 173.8ms +/- 0.6% 173.8ms +/- 0.6% spectral-norm: 1.8% *slower* 122.6ms +/- 0.6% 124.8ms +/- 1.9% significant regexp: 10.3% faster 296.6ms +/- 0.2% 266.0ms +/- 0.0% significant dna: 10.3% faster 296.6ms +/- 0.2% 266.0ms +/- 0.0% significant string: 0.7% faster 865.4ms +/- 0.5% 859.0ms +/- 0.3% significant base64: 2.1% faster 122.8ms +/- 1.1% 120.2ms +/- 0.5% significant fasta: ?? 205.2ms +/- 0.7% 206.4ms +/- 0.3% not conclusive: might be 0.6% *slower* tagcloud: 1.2% faster 196.8ms +/- 0.8% 194.4ms +/- 1.1% significant unpack-code: 0.2% faster 202.0ms +/- 0.0% 201.6ms +/- 0.6% significant validate-input: 1.6% faster 138.6ms +/- 0.5% 136.4ms +/- 1.4% significant

Eric Seidel (no email)

Comment 5 2007-11-15 02:41:37 PST

It looks like darin noticed similar SunSpider craziness relating to regexp in bug 11231.

Sam Weinig

Comment 6 2007-11-15 11:23:07 PST

Comment on attachment 17289 [details] patch (nearly entirely search/replace) Needs a changelog. r=me.

Eric Seidel (no email)

Comment 7 2007-11-15 16:15:02 PST

Landed.

Note You need to log in before you can comment on or make changes to this bug.