LGTM if you address these comments:
http://codereview.chromium.org/3435012/diff/1/5 File benchmarks/README.txt (right): http://codereview.chromium.org/3435012/diff/1/5#newcode74 benchmarks/README.txt:74: benchmark to exercise the regexp engine on different input strings. regexp => regular expression? http://codereview.chromium.org/3435012/diff/1/3 File benchmarks/regexp.js (right): http://codereview.chromium.org/3435012/diff/1/3#newcode1 benchmarks/regexp.js:1: // Copyright 2009-2010 the V8 project authors. All rights reserved. We usually just update to 2010. No need for the range. http://codereview.chromium.org/3435012/diff/1/3#newcode36 benchmarks/regexp.js:36: // affect how the regexps match their input. Finally the strings are Double space before "Finally" to be consistent with the style in the rest of the comment. http://codereview.chromium.org/3435012/diff/1/3#newcode40 benchmarks/regexp.js:40: var RegExp = new BenchmarkSuite('RegExp', 910985, [ I take it that you've measured that these changes do not influence the performance on the reference platform? http://codereview.chromium.org/3435012/diff/1/3#newcode59 benchmarks/regexp.js:59: function scrambleString(str, n) { This seems like a weird name - how about computeInputVariants? Maybe also add a comment that explains how the variants are generated? http://codereview.chromium.org/3435012/diff/1/3#newcode60 benchmarks/regexp.js:60: var stringarray = [ str ]; stringarray => variants (to capture more meaning) http://codereview.chromium.org/3435012/diff/1/3#newcode62 benchmarks/regexp.js:62: pos = Math.floor(Math.random() * str.length); Declare your local variables with var (pos, chr). http://codereview.chromium.org/3435012/diff/1/3#newcode302 benchmarks/regexp.js:302: var s57a = scrambleString('fryrpgrq', 40); It's a bit annoying that the iteration count (40 in this case) is repeated - first at the call to scrambleString and later at the for loop that iterates over s57a. Maybe the scrambleString should really return an object that encapsulates the iteration count too? http://codereview.chromium.org/3435012/diff/1/3#newcode1743 benchmarks/regexp.js:1743: function runRegExpBenchmark() { Maybe just rename this to run? It seems like the term RegExpBenchmark is repeated enough. At the call sites it would still be regexpBenchmark.run() which looks nice. http://codereview.chromium.org/3435012/diff/1/3#newcode1760 benchmarks/regexp.js:1760: this.runRegExpBenchmark = runRegExpBenchmark; this.run = run; http://codereview.chromium.org/3435012/diff/1/2 File benchmarks/revisions.html (right): http://codereview.chromium.org/3435012/diff/1/2#newcode30 benchmarks/revisions.html:30: benchmark to exercise the regexp engine on different input strings.</p> regexp => regular expression http://codereview.chromium.org/3435012/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
