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. On 2010/09/19 12:45:17, Kasper Lund wrote:
regexp => regular expression?
Done. 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. On 2010/09/19 12:45:17, Kasper Lund wrote:
We usually just update to 2010. No need for the range.
Done. There seem to be as many ranges as non-ranges in our copyright headers though. http://codereview.chromium.org/3435012/diff/1/3#newcode36 benchmarks/regexp.js:36: // affect how the regexps match their input. Finally the strings are On 2010/09/19 12:45:17, Kasper Lund wrote:
Double space before "Finally" to be consistent with the style in the
rest of the
comment.
Done. http://codereview.chromium.org/3435012/diff/1/3#newcode40 benchmarks/regexp.js:40: var RegExp = new BenchmarkSuite('RegExp', 910985, [ On 2010/09/19 12:45:17, Kasper Lund wrote:
I take it that you've measured that these changes do not influence the performance on the reference platform?
Good catch. Having checked now, I know that they don't. http://codereview.chromium.org/3435012/diff/1/3#newcode59 benchmarks/regexp.js:59: function scrambleString(str, n) { On 2010/09/19 12:45:17, Kasper Lund wrote:
This seems like a weird name - how about computeInputVariants? Maybe
also add a
comment that explains how the variants are generated?
Done. http://codereview.chromium.org/3435012/diff/1/3#newcode60 benchmarks/regexp.js:60: var stringarray = [ str ]; On 2010/09/19 12:45:17, Kasper Lund wrote:
stringarray => variants (to capture more meaning)
Done. http://codereview.chromium.org/3435012/diff/1/3#newcode62 benchmarks/regexp.js:62: pos = Math.floor(Math.random() * str.length); On 2010/09/19 12:45:17, Kasper Lund wrote:
Declare your local variables with var (pos, chr).
Done. http://codereview.chromium.org/3435012/diff/1/3#newcode302 benchmarks/regexp.js:302: var s57a = scrambleString('fryrpgrq', 40); On 2010/09/19 12:45:17, Kasper Lund wrote:
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?
I thought about having the number of iterations in a less redundant manner. I am reusing the same variant array for loops of different sizes though. http://codereview.chromium.org/3435012/diff/1/3#newcode1743 benchmarks/regexp.js:1743: function runRegExpBenchmark() { On 2010/09/19 12:45:17, Kasper Lund wrote:
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.
Done. http://codereview.chromium.org/3435012/diff/1/3#newcode1760 benchmarks/regexp.js:1760: this.runRegExpBenchmark = runRegExpBenchmark; On 2010/09/19 12:45:17, Kasper Lund wrote:
this.run = run;
Done. 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> On 2010/09/19 12:45:17, Kasper Lund wrote:
regexp => regular expression
Done. http://codereview.chromium.org/3435012/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
