ruby-dev

前田です。

2012年11月12日 14:58 SASADA Koichi <[email protected]>:
> 　この件，私が実装しても shugo さんがされても良いかと思います．ただ，話
> の順番がおかしい，という気がしています．refinement の導入は，
> （refinement の関係の無い部分での）コストが十分に低い，という前提があっ
> たと思います．しかし，今回の件は，この前提をひっくり返している，もしくは
> ひっくり返す可能性があります．

すみません、いったんrevertしました。
テストはskipする状態で残してありますが、このテストが通らない場合はmodule_eval
のブロックでrefinementsを有効にする機能は削るつもりです。

> 　動くので「大問題」ではないと思いますが，このままリリースするには抵抗が
> あります．

リリースするまでに性能の問題がクリアできない場合は機能を削ることを考えて
いましたが、ささださんの作業を考えると一時的にでも入れるべきではなかった
ですね。すみません。

>> * refineした時にrefine対象クラスにメソッドがない時はどうするか
>>   メソッドがない時も常にrefineされたことを示すmethod entryを追加する?
>
> 　その点，私もあのあと気づいたのですが，refine されるメソッドは追加する
> と良いと思います．

追加しない場合、メソッド探索失敗時もrefinementsの探索パスを通すことに
なるかなと思いますが、後でrefine対象のクラスにメソッドが追加されたりすると
まずそうなので、やっぱりrefineした時点でメソッドを追加しておかないと
まずそうです。

> （そもそも，refine されるべきメソッドがないのに refine できる，って仕様
> はどうなの，という気もしましたが．まぁ，そういうシチュエーションもあるの
> かなぁ）

refineの対象はメソッドではなくクラスなので、新規メソッドの追加も仕様と
したいと考えています。
具体的なユースケースとしては、例えばRSpecのshouldのようなものがあります。

>> * refineされたメソッドの呼出時にrb_call_info_tにどういう情報を格納するか
>>   Twitterでは、refineされたことを示すmethod entryを入れておくということでしたが、
>>   今のvm_call_methodの構造だとrb_call_info_tの中身を書き換えてgotoするような
>>   形になっているので、どういう変更を意図されているのかよくわかりませんでした。
>
> 　goto は無いですが，この辺は特に変更は不要な気がします．

vm_call_methodに分岐を増やして、

              case VM_METHOD_TYPE_REFINED:{
                rb_call_info_t cie = *ci;
                ci = &cie;

                ci->me = (refinementsから探索したmethod entry、または元のクラスのmethod entry);
                if (ci->me != 0) {
                    goto normal_method_dispatch;
                }
                else {
                    /* refinementsからメソッドが見つからず、元のクラスにもメソッド定義がない場合 */
                    goto start_method_dispatch;
                }

とするようなイメージで合っていますか?

refinementsから探索したmethod entryは通常のインラインキャッシュには
載らないのですよね?
# できれば別のところにキャッシュを置いた方がいいかもしれないですけど。

> 　結論が出たら，私が実装しますかね．

意図を正しく理解できているか不安ではありますが、ささださんは他にも作業が
たくさんありそうなので、いったん私がやってみましょうか?

あと、本題と関係ありませんが、コードを読んでいて以下の疑問がありました。

* rb_iseq_t::ic_entriesはまだ使われていますか?
* callinfoとcall_infoで名前の付け方に揺れがあるのは意図的ですか?

-- 
Shugo Maeda

Thread

Prev Next

In This Thread

Prev Next