[runtime] split up loops with HandleScopes

[runtime] split up loops with HandleScopes

HandleScopes in for-loops are rather expensive and pose a significant overhead
to some builtin/runtime-functions. The FOR_WITH_HANDLE_SCOPE macro is used to
only create a new HandleScope every 1024th iteration.

BUG=

Committed: https://crrev.com/25d36eacf20f72bf6ba19b1475c0e4f56f95d1a2
Cr-Commit-Position: refs/heads/master@{#34856}

[Camillo Bruni, 2016-03-11 17:49:17]

cbruni@chromium.org changed reviewers:
+ ishell@chromium.org

[Camillo Bruni, 2016-03-11 17:49:18]

PTAL

While micro-benchmarking StringSplit I found that not creating a new HandleScope
on every iteration gave a 4% performance increase.
It's a bit messy with the macro if you make typo...

WDYT?

[Yang, 2016-03-11 19:06:03]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 19:06:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785403002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785403002/20001

[commit-bot: I haz the power, 2016-03-11 19:13:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 19:13:04]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_compil...)

[Igor Sheludko, 2016-03-14 08:36:16]

lgtm with a nit:

https://codereview.chromium.org/1785403002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1785403002/diff/20001/src/builtins.cc#newcode...
src/builtins.cc:1155: FOR_WITH_HANDLE_SCOPE(isolate, int, j = 0, j, j < n, j =
j, {
I think something like (void)0 is better than j = j here.

[Camillo Bruni, 2016-03-14 09:29:23]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-03-14 09:29:24]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1785403002/#ps40001
(title: "addressing nits and fixing break-issue")

[commit-bot: I haz the power, 2016-03-14 09:29:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785403002/40001

[commit-bot: I haz the power, 2016-03-14 09:32:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-14 09:32:13]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12297)

[Yang, 2016-03-14 13:44:18]

https://codereview.chromium.org/1785403002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1785403002/diff/60001/src/isolate.h#newcode184
src/isolate.h:184: loop_var_type init;                                          
         \
Like discussed offline, we can probably implement this by having the HandleScope
outside of the for-loop and have a ResetHandleScope inside the for-loop.

The ResetHandleScope will need to be implemented. It looks at the HandleScope
height and resets it beyond a given limit.

[Camillo Bruni, 2016-03-14 16:05:56]

Apparently I forgot to disable handle scope zapping, after that the results
aren't that clear anymore:

// ORIGINAL CASE
string 1*2                               x           53'189 ops/sec ±0.58% (92
runs sampled)
string 1*10                              x           32'036 ops/sec ±0.49% (88
runs sampled)
string 1*100                             x            5'557 ops/sec ±1.13% (90
runs sampled)
string 1*1000                            x              640 ops/sec ±0.92% (89
runs sampled)
string 10*2                              x           40'876 ops/sec ±0.61% (89
runs sampled)
string 10*10                             x           17'700 ops/sec ±0.59% (85
runs sampled)
string 10*100                            x            2'353 ops/sec ±0.91% (89
runs sampled)
string 100*2                             x           41'182 ops/sec ±0.70% (91
runs sampled)
string 100*10                            x           18'827 ops/sec ±0.79% (89
runs sampled)
string 100*100                           x            2'463 ops/sec ±0.65% (84
runs sampled)

// WITH THE CL
string 1*2                               x           53'681 ops/sec ±0.49% (89
runs sampled)
string 1*10                              x           33'024 ops/sec ±0.41% (95
runs sampled)
string 1*100                             x            6'223 ops/sec ±0.97% (91
runs sampled)
string 1*1000                            x              667 ops/sec ±1.01% (87
runs sampled)
string 10*2                              x           41'296 ops/sec ±0.46% (93
runs sampled)
string 10*10                             x           18'217 ops/sec ±0.55% (95
runs sampled)
string 10*100                            x            2'436 ops/sec ±0.45% (91
runs sampled)
string 100*2                             x           41'237 ops/sec ±0.54% (92
runs sampled)
string 100*10                            x           19'130 ops/sec ±0.35% (87
runs sampled)
string 100*100                           x            2'676 ops/sec ±0.41% (93
runs sampled)

So in the worst case we only gain code complexity (or a rough 1% on string
split), and in the best case roughly 4%.
I tried doing a more elegant solution by resetting the current scope, but that
seems to not work as well as the dead-ugly MACRO.

So I am not sure this is really worth the effort anymore...

[Camillo Bruni, 2016-03-16 11:46:21]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-16 11:46:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785403002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785403002/80001

[Camillo Bruni, 2016-03-16 11:47:24]

PTAL again.
As discussed offline, I removed the macro from non-critical paths and left it
only for runtime and builtin functions.

[commit-bot: I haz the power, 2016-03-16 11:48:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-16 11:48:59]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2994)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/3003)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[Yang, 2016-03-16 12:16:10]

https://codereview.chromium.org/1785403002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1785403002/diff/80001/src/isolate.h#newcode190
src/isolate.h:190: for (; limit_check && loop_var < for_with_handle_limit;
increment) { \
this only works as expected if "init" is "loop_var = 0" and "increment" is
"loop_var++". Can we just remove the loop_var and increment args and hard code
that into the macro to avoid abuse?

[Yang, 2016-03-16 12:16:41]

https://codereview.chromium.org/1785403002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1785403002/diff/80001/src/isolate.h#newcode190
src/isolate.h:190: for (; limit_check && loop_var < for_with_handle_limit;
increment) { \
On 2016/03/16 12:16:10, Yang wrote:
> this only works as expected if "init" is "loop_var = 0" and "increment" is
> "loop_var++". Can we just remove the loop_var and increment args and hard code
> that into the macro to avoid abuse?

I meant to say "remove init and increment args".

[Camillo Bruni, 2016-03-16 15:54:53]

https://codereview.chromium.org/1785403002/diff/80001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1785403002/diff/80001/src/isolate.h#newcode190
src/isolate.h:190: for (; limit_check && loop_var < for_with_handle_limit;
increment) { \
On 2016/03/16 at 12:16:41, Yang wrote:
> On 2016/03/16 12:16:10, Yang wrote:
> > this only works as expected if "init" is "loop_var = 0" and "increment" is
> > "loop_var++". Can we just remove the loop_var and increment args and hard
code
> > that into the macro to avoid abuse?
> 
> I meant to say "remove init and increment args".

from what I see it should work no matter what the start value is and
independently from how you increment. You might just create a couple of
HandleScopes too much in the worst case (if the init value is > 1024).

I prefer having this as close as possible to the for-loop header, since for
instance we don't always have "i" as the loop-variable (depending on the
nesting).

[Yang, 2016-03-17 08:56:45]

LGTM with a comment.

https://codereview.chromium.org/1785403002/diff/100001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1785403002/diff/100001/src/isolate.h#newcode185
src/isolate.h:185: loop_var_type for_with_handle_limit = 0;                     
         \
Wouldn't it be better to have
loop_var_type for_with_handle_limit = loop_var
here? In case loop_var is not initialized to 0.

[Camillo Bruni, 2016-03-17 12:17:48]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-03-17 12:17:48]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1785403002/#ps120001
(title: "addressing comment")

[commit-bot: I haz the power, 2016-03-17 12:17:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785403002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785403002/120001

[commit-bot: I haz the power, 2016-03-17 12:41:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-17 12:43:00]

Description was changed from

==========
[runtime] split up loops with HandleScopes

HandleScopes in for-loops are rather expensive and pose a significant overhead
to some builtin/runtime-functions. The FOR_WITH_HANDLE_SCOPE macro is used to
only create a new HandleScope every 1024th iteration.

BUG=
==========

to

==========
[runtime] split up loops with HandleScopes

HandleScopes in for-loops are rather expensive and pose a significant overhead
to some builtin/runtime-functions. The FOR_WITH_HANDLE_SCOPE macro is used to
only create a new HandleScope every 1024th iteration.

BUG=

Committed: https://crrev.com/25d36eacf20f72bf6ba19b1475c0e4f56f95d1a2
Cr-Commit-Position: refs/heads/master@{#34856}
==========

[commit-bot: I haz the power, 2016-03-17 12:43:02]

Patchset 7 (id:??) landed as
https://crrev.com/25d36eacf20f72bf6ba19b1475c0e4f56f95d1a2
Cr-Commit-Position: refs/heads/master@{#34856}

[Camillo Bruni, 2016-03-17 14:37:55]

https://codereview.chromium.org/1785403002/diff/100001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1785403002/diff/100001/src/isolate.h#newcode185
src/isolate.h:185: loop_var_type for_with_handle_limit = 0;                     
         \
On 2016/03/17 at 08:56:45, Yang wrote:
> Wouldn't it be better to have
> loop_var_type for_with_handle_limit = loop_var
> here? In case loop_var is not initialized to 0.

ah right, that makes sense.