[turbofan] Access merge environment map more efficiently

[turbofan] Access merge environment map more efficiently

Committed: https://crrev.com/8590e8d4d5fca9bee6613f55761af000d5fd1ddb
Cr-Commit-Position: refs/heads/master@{#41481}

[Leszek Swirski, 2016-11-22 17:53:44]

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

[commit-bot: I haz the power, 2016-11-22 17:53:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 18:18:56]

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

[commit-bot: I haz the power, 2016-11-22 18:18:57]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2016-12-01 14:42:27]

leszeks@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Leszek Swirski, 2016-12-01 14:42:27]

Hi Michi,

I noticed some small inefficiencies in the way we were accessing the merge
environments in BGB, so I went ahead and fixed them. Can you take a look please?

[Michael Starzinger, 2016-12-01 14:47:48]

LGTM with a question.

https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1899: auto it =
merge_environments_.find(current_offset);
question: Any reason to use an iterator here and a reference type below? I would
vote for sticking to one or the other in both functions for the sake of
readability.

[Leszek Swirski, 2016-12-01 14:52:36]

I'll wait for a reply to my reply before submitting.

https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1899: auto it =
merge_environments_.find(current_offset);
On 2016/12/01 14:47:48, Michael Starzinger wrote:
> question: Any reason to use an iterator here and a reference type below? I
would
> vote for sticking to one or the other in both functions for the sake of
> readability.

Indeed there is: the find+iterator does not create a new entry in the map if one
didn't exist already, while the array-style accessor does. I could use insert()
below to also use an iterator if you want?

[Michael Starzinger, 2016-12-01 15:00:51]

https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1899: auto it =
merge_environments_.find(current_offset);
On 2016/12/01 14:52:36, Leszek Swirski wrote:
> On 2016/12/01 14:47:48, Michael Starzinger wrote:
> > question: Any reason to use an iterator here and a reference type below? I
> would
> > vote for sticking to one or the other in both functions for the sake of
> > readability.
> 
> Indeed there is: the find+iterator does not create a new entry in the map if
one
> didn't exist already, while the array-style accessor does. I could use
insert()
> below to also use an iterator if you want?

Yes, I would prefer that. Thanks!

[Leszek Swirski, 2016-12-01 15:56:28]

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

[commit-bot: I haz the power, 2016-12-01 15:56:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Leszek Swirski, 2016-12-01 15:59:05]

On 2016/12/01 15:00:51, Michael Starzinger wrote:
>
https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
> File src/compiler/bytecode-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
> src/compiler/bytecode-graph-builder.cc:1899: auto it =
> merge_environments_.find(current_offset);
> On 2016/12/01 14:52:36, Leszek Swirski wrote:
> > On 2016/12/01 14:47:48, Michael Starzinger wrote:
> > > question: Any reason to use an iterator here and a reference type below? I
> > would
> > > vote for sticking to one or the other in both functions for the sake of
> > > readability.
> > 
> > Indeed there is: the find+iterator does not create a new entry in the map if
> one
> > didn't exist already, while the array-style accessor does. I could use
> insert()
> > below to also use an iterator if you want?
> 
> Yes, I would prefer that. Thanks!

Done. Not sure if this is more readable, what do you think?

[commit-bot: I haz the power, 2016-12-01 16:20:28]

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

[commit-bot: I haz the power, 2016-12-01 16:20:29]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-12-01 17:00:43]

https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2521313002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1899: auto it =
merge_environments_.find(current_offset);
On 2016/12/01 15:00:51, Michael Starzinger wrote:
> On 2016/12/01 14:52:36, Leszek Swirski wrote:
> > On 2016/12/01 14:47:48, Michael Starzinger wrote:
> > > question: Any reason to use an iterator here and a reference type below? I
> > would
> > > vote for sticking to one or the other in both functions for the sake of
> > > readability.
> > 
> > Indeed there is: the find+iterator does not create a new entry in the map if
> one
> > didn't exist already, while the array-style accessor does. I could use
> insert()
> > below to also use an iterator if you want?
> 
> Yes, I would prefer that. Thanks!

As discussed offline: Either is fine with me. Take your pick. :)

[Leszek Swirski, 2016-12-05 10:42:06]

The CQ bit was checked by leszeks@chromium.org

[Leszek Swirski, 2016-12-05 10:42:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2521313002/#ps40001
(title: "Revert "Replace array-like access with insert+iterator"")

[commit-bot: I haz the power, 2016-12-05 10:42:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 11:09:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480934526908850,
"parent_rev": "8c5e2c5e937a1334c9489377539377549621ff93", "commit_rev":
"198bc3f1027fdefaa7e35269198be2833d25ae5d"}

[commit-bot: I haz the power, 2016-12-05 11:09:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-05 11:10:10]

Description was changed from

==========
[turbofan] Access merge environment map more efficiently
==========

to

==========
[turbofan] Access merge environment map more efficiently

Committed: https://crrev.com/8590e8d4d5fca9bee6613f55761af000d5fd1ddb
Cr-Commit-Position: refs/heads/master@{#41481}
==========

[commit-bot: I haz the power, 2016-12-05 11:10:11]

Patchset 3 (id:??) landed as
https://crrev.com/8590e8d4d5fca9bee6613f55761af000d5fd1ddb
Cr-Commit-Position: refs/heads/master@{#41481}