I pulled that work into a branch, and I think we should definitely treat it with serious suspicion. In addition to making major changes to the form of the code, it very possibly has a performance impact which we need to benchmark. I think we consider this branch to be extremely experimental and exploratory, and we don't feel compelled to merge it unless we think it's a real benefit.
I'm really coming to the conclusion that the problem with fill_params isn't just that it's too large and ugly, but that it's tasked with too much stuff. Our PCC algorithm, and the promises that we make about all the different types and configurations of parameters and arguments is just too complicated. For instance: 1) If we add a new PIR op get_signature. Then we can tell IMCC to use the new op instead of get_params in this situation: .sub foo .param pmc sig :call_sig In these cases, we can call the new op instead of get_params, never call fill_params, and cut ~10 lines of code out of it. For functions that use this feature, or HLLs that do their own argument processing and sig unpacking, this could be a major win. 1a) For that matter, we could replace get_params with a handful of new ops: get_pmc, get_pmc_slurpy, get_x_named, get_x_optional, etc. Each one would make a single transaction with the signature object. In this case, fill_params evaporates entirely, and the onus is put onto the PIR compiler to generate the argument unpacking code instead of get_params/fill_params. A huge benefit here is that arguments could be retrieved *lazily*, only if they are needed in a particular codepath. What this does do, however, is insert PIR op dispatches between the processing of individual arguments (which is bad for our current naive runcores), but each op becomes extremely simple compared to current fill_params. 2) We know at the very beginning of fill_params whether we need to do error checking or not. We can break fill_params up into two variants: One that does no error checking at all, which does not need to check err_check in tight loops, and one that does full error checking. Looking at the algorithm, I think we can define the later in terms of the former, by doing some input checking first, then do err_check-less fill_params, followed by output checking. This will reduce the size of all functions involved *and* will be a small performance improvement for the common case. 3) If we did not do automatic spill-over between named and positional parameters, we could easily cut fill_params in half. fill_params_positional would only need to be called if the signature contains positionals, and fill_params_named would only need to be called if the signature contains named elements. This would cut fill_params in half, and maybe be an optimization for the common case where only positional or only named parameters were used. I realize that this represents the most major behavior change of all my suggestions, but I do believe that in the long run the performance benefits of not checking for parameter spill-over would be worth it. Simply keeping fill_params as it is now, but breaking it up into smaller chunks may have a small readability/maintainability benefit if done correctly (I don't judge whether or not it was done so by the GCI task), but real benefits are had by changing the algorithm itself. I think we can easily apply changes like (1) and (2) above. (1a) is more exploratory, but should be transparent to an HLL. (3) obviously requires a deprecation cycle. Thanks, --Andrew Whitworth On Fri, Jan 7, 2011 at 7:15 AM, Vasily Chekalkin <[email protected]> wrote: > Hello. > > I tend to agree with Nick. > > -- > Bacek > > On Fri, Jan 7, 2011 at 11:04 PM, Nick Wellnhofer <[email protected]> wrote: >> >> I've already mentioned on IRC that I'm skeptical about the GCI task to >> remove 100 lines from the fill_params function. The task has now been >> completed and the result looks like this: >> >> https://github.com/rofflwaffls/parrot/commit/18d905aec2c0c352c42dd98b7dcf1a0640270929 >> >> Now we have 270 lines added and some static functions with up to 9 >> arguments. fill_params still has 300 lines of code. In my opinion this is a >> lot uglier than a 400 lines function. >> >> Nick >> _______________________________________________ >> http://lists.parrot.org/mailman/listinfo/parrot-dev >> > _______________________________________________ > http://lists.parrot.org/mailman/listinfo/parrot-dev > _______________________________________________ http://lists.parrot.org/mailman/listinfo/parrot-dev
