[ruby-dev:39960] Re: Fwd: [ruby-cvs:33451] Ruby:r26235 (trunk): * trace.h: new file. wraps tracing mechanisms.

遠藤です。

API レベルで見える追加のある、一見するとでかいコミットがいきなり入った
ので、びっくりしました。一応 ML での予告が欲しいです。

それはともかく、ざっとコードレビューの真似事をしてみました。


○ LIKELY / UNLIKELY の名前がまずい (重大)

RB_LIKELY など、他のライブラリと衝突しなさそうな名前にする必要があります。

また、ruby.h に書かれるということは、LIKELY / UNLIKELY は公開 API なの
でしょうか (個人的には、ruby.h に書かれているだけでは公開 API ではないと
思っていますが、Yugui さんの考えではそうではなさそうなので) 。
とりあえず /** @internal */ とかつけるとどうでしょう。


○ RUBY_EVENT_RESCUE が追加された (やや重大)

これは dtrace に関係なく set_trace_func でも見える API レベルの変更です。

  $ ./ruby -e '
   set_trace_func(proc {|name, | p name })
   begin
     raise
   rescue
   end
  '
  "c-return"
  "line"
  *snip*
  "c-return"
  "unknown"  # <== この行！！

3 点あります。

  - API レベルの仕様変更だけど OK ？ 今回のコミットとは別に議論すべき
    ではないか
  - unknown という名前は直すべき
  - これで得られる binding は使っても大丈夫？ (SEGV とかしない？)


○ UNLIKELY が使われているところと使われていないところがある

gc.c の中に

  if (UNLIKELY(TRACE_RAISE_ENABLED())) FIRE_RAISE_FATAL();

という箇所と、

  if (TRACE_OBJECT_FREE_ENABLED()) FIRE_OBJECT_FREE(rb_obj_id(obj));

という箇所があります。この違いは意図的でしょうか。それとも単なる付け
忘れ？ 他にもいくつか UNLIKELY がついていない probe がありました。


○ rb_class2name_without_alloc はなぜ必要か

variable.c に rb_class2name_without_alloc という関数が追加されています
が、grep しただけではどこで使われているのかわかりませんでした。
これは今回の変更で一緒に追加する必要があったのでしょうか。


○ thread_pthread.c に probe を入れるのは OK ？

thread_pthread.c にいろいろ入れるのはやや気持ち悪いです。
ささださんが OK と言えば OK だと思いますが。

異常終了の exit の直前に FIRE_RAISE_FATAL() を入れたかっただけのように
見えますが、似た文脈で exit を読んでいる箇所は regcomp.c などにもちらほら
あるようです。
ruby_fatal_exit とか作って、従来異常終了の文脈で exit を読んでいる箇所
では全部 ruby_fatal_exit を呼ぶようにしたらどうでしょう。


○ probe 拡張ライブラリが荒削り

usa さんの突っ込みに同意です。 [ruby-dev:39958]


それにしても、gcc に AOP があればなあと思うような変更ですね。

-- 
Yusuke ENDOH <[e-mail]>