For Micro-benchmarks for 'with'

For Micro-benchmarks for 'with'

R=arv@chromium.org, mstarzinger@chromium.org
BUG=v8:4131
LOG=n

Committed: https://crrev.com/f62d5ce9122b86527d97c11c82cc1d22327f563d
Cr-Commit-Position: refs/heads/master@{#28714}

[Michael Hablich, 2015-05-28 07:48:15]

Please advised this is a very early draft. Erik, it would be great if you can
point me in the right direction.

[Michael Starzinger, 2015-05-28 13:58:58]

LGTM. But I am not familiar with the perf-test suite and the policies
surrounding that at all. Better wait for Arv's approval as well.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/JSTests.json
File test/js-perf-test/JSTests.json (right):

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/JSTests.j...
test/js-perf-test/JSTests.json:111: "name": "PreES6",
nit: Should we just call this (and the directory) "ES5" instead?

[arv (Not doing code reviews), 2015-05-28 15:18:34]

The name of this suite is a bit questionable. with was added in ES2 (3?). A
better name of this might just be "With"?

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
File test/js-perf-test/PreES6/with.js (right):

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:1: // Copyright 2014 the V8 project authors.
All rights reserved.
15

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:19: var VALUE_OF_PROPERTY = "Simply a string";
Don't mix and match " and '. Stick to one.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:24: objectUnderTest = {"First":
VALUE_OF_PROPERTY};
objectUnderTest = {first: VALUE_OF_PROPERTY};

Capitalized names are for constructors.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:28: with(objectUnderTest) {
with (

just like, if, else for, while, catch etc.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:41: objectUnderTestExtended =
Object.create(objectUnderTest);
Or

objectUnderTestExtended = {
  __proto__: objectUnderTest,
  Second: "Another string"
};

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:70: }
Do we want to test assignment too?

with (objectUnderTest) {
  first = SOME_OTHER_VALUE;
}

[arv (Not doing code reviews), 2015-05-28 15:20:37]

That bug does not exist for me.

Don't you need to add LOG=N to CL description?

[Michael Hablich, 2015-05-29 14:13:22]

On 2015/05/28 15:20:37, arv wrote:
> That bug does not exist for me.
> 
> Don't you need to add LOG=N to CL description?

Added you to CC. Michael also mentioned that PreES6 is not a good name. I agree
too. How about naming the suite 'Scope'?

[Michael Hablich, 2015-05-29 14:14:50]

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/JSTests.json
File test/js-perf-test/JSTests.json (right):

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/JSTests.j...
test/js-perf-test/JSTests.json:111: "name": "PreES6",
On 2015/05/28 13:58:58, Michael Starzinger wrote:
> nit: Should we just call this (and the directory) "ES5" instead?

*agree*. Current candidates are 'With' and 'Scope'.

[Michael Hablich, 2015-05-29 14:36:59]

PTAL

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
File test/js-perf-test/PreES6/with.js (right):

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:1: // Copyright 2014 the V8 project authors.
All rights reserved.
On 2015/05/28 15:18:33, arv wrote:
> 15

Done.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:5: new BenchmarkSuite('With', [1000], [
Does anybody have a clue what would be a good reference?

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:19: var VALUE_OF_PROPERTY = "Simply a string";
On 2015/05/28 15:18:33, arv wrote:
> Don't mix and match " and '. Stick to one.

Done.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:24: objectUnderTest = {"First":
VALUE_OF_PROPERTY};
On 2015/05/28 15:18:33, arv wrote:
> objectUnderTest = {first: VALUE_OF_PROPERTY};
> 
> Capitalized names are for constructors.

Done.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:28: with(objectUnderTest) {
On 2015/05/28 15:18:33, arv wrote:
> with (
> 
> just like, if, else for, while, catch etc.

Done.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:41: objectUnderTestExtended =
Object.create(objectUnderTest);
On 2015/05/28 15:18:33, arv wrote:
> Or
> 
> objectUnderTestExtended = {
>   __proto__: objectUnderTest,
>   Second: "Another string"
> };

IMO mine is more readable.

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:70: }
On 2015/05/28 15:18:33, arv wrote:
> Do we want to test assignment too?
> 
> with (objectUnderTest) {
>   first = SOME_OTHER_VALUE;
> }

Done.

[arv (Not doing code reviews), 2015-05-29 14:43:39]

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
File test/js-perf-test/PreES6/with.js (right):

https://codereview.chromium.org/1153363002/diff/1/test/js-perf-test/PreES6/wi...
test/js-perf-test/PreES6/with.js:41: objectUnderTestExtended =
Object.create(objectUnderTest);
On 2015/05/29 14:36:59, Hablich wrote:
> On 2015/05/28 15:18:33, arv wrote:
> > Or
> > 
> > objectUnderTestExtended = {
> >   __proto__: objectUnderTest,
> >   Second: "Another string"
> > };
> 
> IMO mine is more readable.

IMO does not apply to style =P

https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Array_...

https://codereview.chromium.org/1153363002/diff/40001/test/js-perf-test/Scope...
File test/js-perf-test/Scope/with.js (right):

https://codereview.chromium.org/1153363002/diff/40001/test/js-perf-test/Scope...
test/js-perf-test/Scope/with.js:46: objectUnderTestExtended.Second = 'Another
string';
s/Second/second/

https://codereview.chromium.org/1153363002/diff/40001/test/js-perf-test/Scope...
test/js-perf-test/Scope/with.js:66: var First = 'Not correct';
s/First/first/

This one is not just style, the name here does not match the property name which
I believe was the intention.

[Michael Hablich, 2015-05-29 14:48:57]

https://codereview.chromium.org/1153363002/diff/40001/test/js-perf-test/Scope...
File test/js-perf-test/Scope/with.js (right):

https://codereview.chromium.org/1153363002/diff/40001/test/js-perf-test/Scope...
test/js-perf-test/Scope/with.js:46: objectUnderTestExtended.Second = 'Another
string';
On 2015/05/29 14:43:39, arv wrote:
> s/Second/second/

Done.

https://codereview.chromium.org/1153363002/diff/40001/test/js-perf-test/Scope...
test/js-perf-test/Scope/with.js:66: var First = 'Not correct';
On 2015/05/29 14:43:39, arv wrote:
> s/First/first/
> 
> This one is not just style, the name here does not match the property name
which
> I believe was the intention.

Done.

[arv (Not doing code reviews), 2015-05-29 14:54:44]

LGTM

https://codereview.chromium.org/1153363002/diff/60001/test/js-perf-test/JSTes...
File test/js-perf-test/JSTests.json (right):

https://codereview.chromium.org/1153363002/diff/60001/test/js-perf-test/JSTes...
test/js-perf-test/JSTests.json:115: "flags": ["--turbo"],
Would it not be better to not have this flag to see how the performance is
impacted once we start using TF for with?

[Michael Hablich, 2015-05-29 15:02:02]

On 2015/05/29 14:54:44, arv wrote:
> LGTM
> 
>
https://codereview.chromium.org/1153363002/diff/60001/test/js-perf-test/JSTes...
> File test/js-perf-test/JSTests.json (right):
> 
>
https://codereview.chromium.org/1153363002/diff/60001/test/js-perf-test/JSTes...
> test/js-perf-test/JSTests.json:115: "flags": ["--turbo"],
> Would it not be better to not have this flag to see how the performance is
> impacted once we start using TF for with?

Good question. TF is currently already optimizing 'with' theoretically.

Currently there is not really a difference in the results.

mstarzinger@ what is your suggestion?

[Michael Starzinger, 2015-05-29 15:04:20]

On 2015/05/29 15:02:02, Hablich wrote:
> On 2015/05/29 14:54:44, arv wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1153363002/diff/60001/test/js-perf-test/JSTes...
> > File test/js-perf-test/JSTests.json (right):
> > 
> >
>
https://codereview.chromium.org/1153363002/diff/60001/test/js-perf-test/JSTes...
> > test/js-perf-test/JSTests.json:115: "flags": ["--turbo"],
> > Would it not be better to not have this flag to see how the performance is
> > impacted once we start using TF for with?
> 
> Good question. TF is currently already optimizing 'with' theoretically.
> 
> Currently there is not really a difference in the results.
> 
> mstarzinger@ what is your suggestion?

If we want to measure what is shipping, then I agree with Arv and we should drop
the flag. If we want to always measure TurboFan, then we should keep the flag.
If I had to choose, then I guess the former gives more useful data-points, so +1
for dropping it.

[Michael Hablich, 2015-06-01 07:18:03]

The CQ bit was checked by hablich@chromium.org

[Michael Hablich, 2015-06-01 07:18:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1153363002/#ps80001
(title: "Drop --turbo flag so we measure what we ship")

[commit-bot: I haz the power, 2015-06-01 07:18:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153363002/80001

[commit-bot: I haz the power, 2015-06-01 07:40:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-01 07:40:19]

Patchset 5 (id:??) landed as
https://crrev.com/f62d5ce9122b86527d97c11c82cc1d22327f563d
Cr-Commit-Position: refs/heads/master@{#28714}