Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1040093002:
Change the collection of continuation jumps. (Closed)
|
Created:
5 years, 8 months ago by Kevin Millikin (Google) Modified:
5 years, 8 months ago CC:
reviews_dartlang.org Target Ref:
refs/remotes/git-svn Visibility:
Public. |
DescriptionChange the collection of continuation jumps.
Encapsulate the responsibility for emitting jumps and building continuation
environments in the JumpCollector class. JumpCollectors are now used for
continuations that are not break or continue targets in the source
program (e.g., some of the loop continuations) for uniformity.
BUG=
R=asgerf@google.com
Committed: https://code.google.com/p/dart/source/detail?r=44964
Patch Set 1 #Patch Set 2 : Clean up some comments. #
Total comments: 26
Patch Set 3 : Incorporated review comments, rebased. #
Messages
Total messages: 8 (1 generated)
kmillikin@google.com changed reviewers: + asgerf@google.com, sra@google.com
The existing approach was starting to get unwieldy for the translation of try/finally.
https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right): https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:51: Environment.fresh(Environment other, List<ir.Parameter> parameters) `parameters` is an output parameter here: the caller provides a list that is filled in. I can't think of another nice and simple way to do it. I will strengthen the doc comment to make it clear what is going on.
LGTM! Code in the IR builder is still fairly hard to read, but it has improved a lot. I wish we could make up a better interface/metaphor for the jump collectors, though. The use-site code looks nice, which is a huge improvement. But still, after reading the doc comments and implementation for these classes, I didn't feel I had a good grasp of what they were doing. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (left): https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1943: I think red is my new favorite color. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right): https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:51: Environment.fresh(Environment other, List<ir.Parameter> parameters) On 2015/04/07 09:14:55, kmillikin wrote: > `parameters` is an output parameter here: the caller provides a list that is > filled in. I can't think of another nice and simple way to do it. > > I will strengthen the doc comment to make it clear what is going on. But it's a copy of index2value. Could you just copy index2value after creating the environment? https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:130: /// True if the collector has recorded any jumps to its continuation. has *not* recorded https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:148: /// translating it. Could you start the comment with "All jumps" instead of "Jumps"? I had to re-read the sentence a few times until I realized that Jumps is a noun and not a verb. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:195: final List<Environment> _environments = <Environment>[]; Could we rename this to _invocationEnvironments? It makes the code in _setContinuation more readable. We currently have '_environments' and '_environment' which does nothing to emphasize what is what. As a side effect, it also helps emphasize that the two lists declared here are paired. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:202: /// original environment will not effect the collector. affect https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:242: // the one in scope for the continuation body. '_environment' and 'env' (below) are bad names when we are dealing with multiple environments. Possibly rename '_environment' to '_targetEnvironment'? Or introduce a local alias with a better name. As for 'env' below, how about 'jumpEnv' or 'jumpSiteEnv', 'invocationEnv', 'invocationSiteEnv' or something like that? https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:262: Environment currentEnvironment = _environments[jumpIndex]; Again, something like 'jumpEnvironment' would be better than 'currentEnvironment'. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:264: if (parameters.isNotEmpty) { I think we should make 'parameters' the outer loop and lose all this bailout code. E.g: int varIndex = 0; for (ir.Parameter param in parameters) { varIndex = bindingEnvironment.index2value.indexOf(param, varIndex); arguments.add(new ir.Reference(jumpEnvironment[varIndex])); } https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:786: // avoid inserting a useless continuation parameter. I really like using the environment like this. Before this CL I hadn't even realized we used the environment for this purpose, but now it's a lot more clear. With that said, at a first glance, the lengthy comment suggests that this is an important optimization and that the following lines have something to do with that optimization. Could you emphasize that we just need to extend the environment with an arbitrary placeholder value? Using thenValue sort of happens to generate a better result, but that's not the most important thing happening here. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1653: } I may be nitpicking now, but could we please restore the outer collectors AFTER this if? This seems to violate a stack discipline. This also has the side benfit of moving the jumpTo() closer to the buildTryBlock(). There is a recurring pattern of "build and then jump". This pattern is fairly easy to understand, so it helps a lot if we can recognize easily. In this instance it is surround by some stack-based state manipulation, which is also fairly digestable, but not as much when you can't see at a glance that it's a stack. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1757: } Could we have an else with a comment saying that the body was empty and so nothing needs to be done? Right now it sort of looks like a forgotton case. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1877: environment.extend(null, null); I love this!
https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right): https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:51: Environment.fresh(Environment other, List<ir.Parameter> parameters) On 2015/04/07 13:42:42, asgerf wrote: > On 2015/04/07 09:14:55, kmillikin wrote: > > `parameters` is an output parameter here: the caller provides a list that is > > filled in. I can't think of another nice and simple way to do it. > > > > I will strengthen the doc comment to make it clear what is going on. > > But it's a copy of index2value. Could you just copy index2value after creating > the environment? Yeah, I can do that. I was trying to make it enforced in the API and without having the caller poke around at the internal bits of the environment. Since there is currently only one call site and I don't imagine a whole lot more than that, I'll do as you suggest. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:130: /// True if the collector has recorded any jumps to its continuation. On 2015/04/07 13:42:42, asgerf wrote: > has *not* recorded Done. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:148: /// translating it. On 2015/04/07 13:42:41, asgerf wrote: > Could you start the comment with "All jumps" instead of "Jumps"? > > I had to re-read the sentence a few times until I realized that Jumps is a noun > and not a verb. Done. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:195: final List<Environment> _environments = <Environment>[]; On 2015/04/07 13:42:42, asgerf wrote: > Could we rename this to _invocationEnvironments? > > It makes the code in _setContinuation more readable. We currently have > '_environments' and '_environment' which does nothing to emphasize what is what. > > As a side effect, it also helps emphasize that the two lists declared here are > paired. Done. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:202: /// original environment will not effect the collector. On 2015/04/07 13:42:41, asgerf wrote: > affect Done. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:242: // the one in scope for the continuation body. On 2015/04/07 13:42:41, asgerf wrote: > '_environment' and 'env' (below) are bad names when we are dealing with multiple > environments. > > Possibly rename '_environment' to '_targetEnvironment'? Or introduce a local > alias with a better name. > > As for 'env' below, how about 'jumpEnv' or 'jumpSiteEnv', 'invocationEnv', > 'invocationSiteEnv' or something like that? Done. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:264: if (parameters.isNotEmpty) { On 2015/04/07 13:42:42, asgerf wrote: > I think we should make 'parameters' the outer loop and lose all this bailout > code. E.g: > > int varIndex = 0; > for (ir.Parameter param in parameters) { > varIndex = bindingEnvironment.index2value.indexOf(param, varIndex); > arguments.add(new ir.Reference(jumpEnvironment[varIndex])); > } Done. It is probably better, though not simpler, to compute the parameter indexes as a list of indexes above, then just iterate that rather than repeatedly searching for their index. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:786: // avoid inserting a useless continuation parameter. On 2015/04/07 13:42:42, asgerf wrote: > I really like using the environment like this. Before this CL I hadn't even > realized we used the environment for this purpose, but now it's a lot more > clear. > > With that said, at a first glance, the lengthy comment suggests that this is an > important optimization and that the following lines have something to do with > that optimization. > > Could you emphasize that we just need to extend the environment with an > arbitrary placeholder value? Using thenValue sort of happens to generate a > better result, but that's not the most important thing happening here. I've mentioned that first, though I thought the comment before the asserts was serving that purpose. I've left the long-winded comment in place. It's not intended to make this sound like an important optimization, but I do think it's necessary to justify its safety. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1653: } On 2015/04/07 13:42:42, asgerf wrote: > I may be nitpicking now, but could we please restore the outer collectors AFTER > this if? This seems to violate a stack discipline. > > This also has the side benfit of moving the jumpTo() closer to the > buildTryBlock(). > > There is a recurring pattern of "build and then jump". This pattern is fairly > easy to understand, so it helps a lot if we can recognize easily. > > In this instance it is surround by some stack-based state manipulation, which is > also fairly digestable, but not as much when you can't see at a glance that it's > a stack. That's fine. I can even restore the break and continue collectors in the opposite order to make it really clear :) However, in all the other sites, I have not done exactly what you describe: I have pushed/popped the break and continue collector stacks immediately around visiting the statement in question, and only after that added a jump to the end of the body if necessary. I consciously did it that way to emphasize that the 'scope' of the modified break and continue stacks was exactly the translation of the body. I was worried that the translation of some unrelated term would creep in at the wrong place. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1757: } On 2015/04/07 13:42:41, asgerf wrote: > Could we have an else with a comment saying that the body was empty and so > nothing needs to be done? Right now it sort of looks like a forgotton case. Done.
On 2015/04/07 13:42:42, asgerf wrote: > LGTM! > > Code in the IR builder is still fairly hard to read, but it has improved a lot. > > I wish we could make up a better interface/metaphor for the jump collectors, > though. The use-site code looks nice, which is a huge improvement. But still, > after reading the doc comments and implementation for these classes, I didn't > feel I had a good grasp of what they were doing. I agree with this, but I'm not sure what that metaphor is. They represent a pair of functions: 1. A function of type IrBuilder -> void that has the side effect of emitting a jump. 2. A function of type Environment -> (Environment, Continuation) that computes a target environment and continuation (parameters). So 'collector' isn't right. (And the ForwardJumpCollector doesn't even collect anything anyway.) I don't have a good idea right now for anything better.
https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right): https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:786: // avoid inserting a useless continuation parameter. On 2015/04/08 10:56:59, kmillikin wrote: > On 2015/04/07 13:42:42, asgerf wrote: > > I really like using the environment like this. Before this CL I hadn't even > > realized we used the environment for this purpose, but now it's a lot more > > clear. > > > > With that said, at a first glance, the lengthy comment suggests that this is > an > > important optimization and that the following lines have something to do with > > that optimization. > > > > Could you emphasize that we just need to extend the environment with an > > arbitrary placeholder value? Using thenValue sort of happens to generate a > > better result, but that's not the most important thing happening here. > > I've mentioned that first, though I thought the comment before the asserts was > serving that purpose. I've left the long-winded comment in place. It's not > intended to make this sound like an important optimization, but I do think it's > necessary to justify its safety. You are right, I overlooked the comment before the asserts. https://codereview.chromium.org/1040093002/diff/20001/pkg/compiler/lib/src/cp... pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1653: } On 2015/04/08 10:56:59, kmillikin wrote: > On 2015/04/07 13:42:42, asgerf wrote: > > I may be nitpicking now, but could we please restore the outer collectors > AFTER > > this if? This seems to violate a stack discipline. > > > > This also has the side benfit of moving the jumpTo() closer to the > > buildTryBlock(). > > > > There is a recurring pattern of "build and then jump". This pattern is fairly > > easy to understand, so it helps a lot if we can recognize easily. > > > > In this instance it is surround by some stack-based state manipulation, which > is > > also fairly digestable, but not as much when you can't see at a glance that > it's > > a stack. > > That's fine. I can even restore the break and continue collectors in the > opposite order to make it really clear :) > > However, in all the other sites, I have not done exactly what you describe: I > have pushed/popped the break and continue collector stacks immediately around > visiting the statement in question, and only after that added a jump to the end > of the body if necessary. I consciously did it that way to emphasize that the > 'scope' of the modified break and continue stacks was exactly the translation of > the body. I was worried that the translation of some unrelated term would creep > in at the wrong place. Just don't do breakCollectors.reversed.forEach(restoreJumps). That would be overkill.
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 44964 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
