PEAK XOOPS - kernel/module.php のキャッシュが効いてないバグ in englishin japanese

Archive | RSS |
XOOPS
XOOPS : kernel/module.php のキャッシュが効いてないバグ
Poster : GIJOE on 2007-03-06 03:26:48 (8689 reads)

in englishin japanese
サブメニューの実験をやっていて気づいたのですが、あるモジュールの xoops_version.php が2度も3度も読み込まれてしまいます。xoops_version.phpにロジックが書いてあるモジュールも多いので、二重読込はそのままオーバーヘッドになります。

ただ、kernel/module.php を見ると、loadInfo() メソッド自体はちゃんと$modversionをキャッシュしてますし、getInfo()もそのキャッシュを利用してます。

であれば犯人はどこだろう、と調べてみてビックリしました。モジュールハンドラのモジュールオブジェクトキャッシュが事実上機能してなかったんですね(参照で返すべきところをコピーで返していた、なぜかget()内のローカルstatic変数で持っていた等、かなりボロボロ)。モジュールオブジェクトが次々と作られてしまうため、モジュールオブジェクト毎に、$modversion をキャッシュしても意味がなかったのです。

とりあえず下のようにパッチしてみました。

これで、必要数以上のモジュールオブジェクトが生成されることはなくなり、xoops_version.php の2重読込もなくなったように見えますが、運用上の問題がでるかどうかは分かりません。しばらくこのサイトで、このコードで動かしてみます。

Cube Legacy 2.1RCのソースコードを読む限り、これとまったく同じ問題を抱えているようです。βくらいで気づけば良かったのですが、今からだとちょっと難しいでしょうか。メモリまたは速度に効いてきそうではありますが。


*** module.php.orig     Mon May  1 11:37:28 2006
--- module.php  Tue Mar  6 03:12:49 2007
***************
*** 316,328 ****
       */
      function &get($id)
      {
-         static $_cachedModule_dirname;
-         static $_cachedModule_mid;
          $ret = false;
          $id = intval($id);
          if ($id > 0) {
!             if (!empty($_cachedModule_mid[$id])) {
!                 return $_cachedModule_mid[$id];
              } else {
                  $sql = 'SELECT * FROM '.$this->db->prefix('modules').' WHERE m
id = '.$id;
                  if ($result = $this->db->query($sql)) {
--- 316,326 ----
       */
      function &get($id)
      {
          $ret = false;
          $id = intval($id);
          if ($id > 0) {
!             if (!empty($this->_cachedModule_mid[$id])) {
!                 $ret =& $this->_cachedModule_mid[$id];
              } else {
                  $sql = 'SELECT * FROM '.$this->db->prefix('modules').' WHERE m
id = '.$id;
                  if ($result = $this->db->query($sql)) {
***************
*** 331,338 ****
                          $module =& new XoopsModule();
                          $myrow = $this->db->fetchArray($result);
                          $module->assignVars($myrow);
!                         $_cachedModule_mid[$id] =& $module;
!                         $_cachedModule_dirname[$module->getVar('dirname')] =&
$module;
                          $ret =& $module;
                      }
                  }
--- 329,336 ----
                          $module =& new XoopsModule();
                          $myrow = $this->db->fetchArray($result);
                          $module->assignVars($myrow);
!                         $this->_cachedModule_mid[$id] =& $module;
!                         $this->_cachedModule_dirname[$module->getVar('dirname'
)] =& $module;
                          $ret =& $module;
                      }
                  }
***************
*** 350,360 ****
       */
      function &getByDirname($dirname)
      {
-         static $_cachedModule_mid;
-         static $_cachedModule_dirname;
          $ret = false;
!         if (!empty($_cachedModule_dirname[$dirname])) {
!             $ret = $_cachedModule_dirname[$dirname];
          } else {
              $sql = "SELECT * FROM ".$this->db->prefix('modules')." WHERE dirna
me = '".trim($dirname)."'";
              if ($result = $this->db->query($sql)) {
--- 348,356 ----
       */
      function &getByDirname($dirname)
      {
          $ret = false;
!         if (!empty($this->_cachedModule_dirname[$dirname])) {
!             $ret =& $this->_cachedModule_dirname[$dirname];
          } else {
              $sql = "SELECT * FROM ".$this->db->prefix('modules')." WHERE dirna
me = '".trim($dirname)."'";
              if ($result = $this->db->query($sql)) {
***************
*** 363,370 ****
                      $module =& new XoopsModule();
                      $myrow = $this->db->fetchArray($result);
                      $module->assignVars($myrow);
!                     $_cachedModule_dirname[$dirname] =& $module;
!                     $_cachedModule_mid[$module->getVar('mid')] =& $module;
                      $ret =& $module;
                  }
              }
--- 359,366 ----
                      $module =& new XoopsModule();
                      $myrow = $this->db->fetchArray($result);
                      $module->assignVars($myrow);
!                     $this->_cachedModule_dirname[$dirname] =& $module;
!                     $this->_cachedModule_mid[$module->getVar('mid')] =& $modul
e;
                      $ret =& $module;
                  }
              }
***************
*** 496,503 ****
              return $ret;
          }
          while ($myrow = $this->db->fetchArray($result)) {
!             $module =& new XoopsModule();
!             $module->assignVars($myrow);
              if (!$id_as_key) {
                  $ret[] =& $module;
              } else {
--- 492,506 ----
              return $ret;
          }
          while ($myrow = $this->db->fetchArray($result)) {
!             $mid = intval( $myrow['mid'] ) ;
!             if( !empty( $this->_cachedModule_mid[$mid] ) ) {
!                 $module =& $this->_cachedModule_mid[$mid] ;
!             } else {
!                 $module =& new XoopsModule();
!                 $module->assignVars($myrow);
!                 $this->_cachedModule_mid[$module->getVar('mid')] =& $module;
!                 $this->_cachedModule_dirname[$module->getVar('dirname')] =& $m
odule;
!             }
              if (!$id_as_key) {
                  $ret[] =& $module;
              } else {


Related articles
Printer friendly page Send this story to a friend

Comments list

GIJOE  Posted on 2007/3/29 3:43
Quote:

minahito wrotes:
これ、やっちゃうことにしました。
RC がなんだぁ〜(おい
判断を尊重いたします〜
とりあえず、多くのサイト(いずれもX2)にパッチを当てましたが、どれも正常に動いているみたいですよ。
Legacy2.1へのパッチも、今を外すと辛そうですから。

Quote:
static と global で引っ張った変数には直接リファレンスはとれないのですが、 static $t = array() にして $t['foo'] =& ... は OK みたいです。これは直接リファレンスを収録しているのは array() メモリ空間なので...
私もこれはちょっと勘違いしていたみたいです。ただ、ローカルなstaticに別々に保存してあるのは確かなので、各モジュールについて、最大3個ずつのインスタンスってことですね。

Quote:
2.1 系から本家へレポートしたほうがいいバグもすごくたくさんあるんですが(途中で把握しきれなくなった orz 都度やっとけばよかった)、これは忘れずレポートしたほうがよさそうですね。まぁでも PEAK は読まれているから大丈夫だと思いますが...

少なくともこの件は、同じオブジェクトじゃないのは「気持ち悪い」というレベルで、最悪でもパフォーマンスだけですから、あまり重要じゃないかもしれませんね。
minahito  Posted on 2007/3/28 12:36
これ、やっちゃうことにしました。
RC がなんだぁ〜(おい

static と global で引っ張った変数には直接リファレンスはとれないのですが、 static $t = array() にして $t['foo'] =& ... は OK みたいです。これは直接リファレンスを収録しているのは array() メモリ空間なので...

なので、ハンドラ自体のキャッシュは(現時点では) xoops_gethandler にまかせて、 static で別々にとっている発行オブジェクトのキャッシュをメンバでとる方向(つまり GIJOE's patch)で大丈夫そうです。

2.1 系から本家へレポートしたほうがいいバグもすごくたくさんあるんですが(途中で把握しきれなくなった orz 都度やっとけばよかった)、これは忘れずレポートしたほうがよさそうですね。まぁでも PEAK は読まれているから大丈夫だと思いますが...
GIJOE  Posted on 2007/3/6 18:16
sourceforge.net でのやりとりは追ってませんが、(不毛そうなので

Quote:

http://jp.php.net/manual/ja/language.variables.scope .別の問題ですが
phpマニュアルに
「PHP4を駆動するZend Engine 1では
static変数にリファレンスの値は保持できない」

これは初めて知りました。(勉強になります!)

ただこのケースは、staticにリファレンスが保持できるかどうかだけの問題じゃなかったりします。

少なくとも、従前のコードでは、

最初のgetObjects()で1個。
次のget()で1個。
最後にgetByDirname()で1個。

と、1つのモジュール(dirname)につき、3個のモジュールオブジェクトが作成されていたのを確認しました。

これは、Zend Engine 1 側の不具合うんぬんというより、純粋にコーディングミスだと思います。ZE2以上でも、同様に3個のオブジェクトが作られるはずです。
domifara  Posted on 2007/3/6 16:58 | Last modified
んー、たしかに、staticの使い方がおかしいみたいです
と思ったのですが
Quote:
100%の確証が持てないのが XOOPS2 の恐ろしいところ
鋭い、
どうもphpバージョン違いとかサーバー違いの実動作の検証が必要そうです

というのが、
どこかで聞いたと覚えがあるけどと思い、
過去のトラッカーを探してみました
CVS
http://xoops.cvs.sourceforge.net/xoops/xoops2/kernel/module.php?r1=1.11.2.1&r2=1.11.2.2

たぶん xoops2.0.9からかな?
「モジュールハンドラのキャシュが効いてない」
のクレームに対して、
static を宣言することで直ったかのように書いているのですが?
なぜ、
もともとが
$this->_cachedModule_mid
で維持しようとした

だけど前はキャシュ動作が出来ていないとバグトラッカーに報告がある
phppp (#1044971)さんと、mithyt2 (#989462)さんですね

ソースを見る限り、今見ると原因は謎に思えます

今のphpマニュアルを見ると
&get

&getByDirname

でスコープの違う
static
になるので
それぞれに
&get

&getByDirname
で、別のものを見ていることになるので

(もしかして、前はスコープは?一つだった?)

staticでないと維持できないという実状があるのなら?

staticに維持するところを、別関数一つに
インスタンスの確認とセットをまとめるとか?

でも
たまたま今日そのあたりphpマニュアル見てたら
http://jp.php.net/manual/ja/language.variables.scope .別の問題ですが
phpマニュアルに
「PHP4を駆動するZend Engine 1では
static変数にリファレンスの値は保持できない」
でサーバーの違いも考慮が必要かと
(これは、いつ追加されたのかな?
Zend Engine 1では意図せぬ動きをするらしいと
書いてるから、んーそうなのでしょう)
GIJOE  Posted on 2007/3/6 12:39
むしろ今まで動いていた方が不思議なくらいなので、パッチしちゃっても、大丈夫じゃないかとは思いますが、すでにステージがRCなだけに…
minahito  Posted on 2007/3/6 9:45
のおおおっ orz

普通に考えればパッチしても問題ないはずですが、100%の確証が持てないのが XOOPS2 の恐ろしいところ...;;

preload で xoops_gethandler() が返却するカーネルを差し替えることができる仕様を使用して、テスター向けに preload を配布しましょうか...
(テスター向けということなら差分ファイル配ったほうが早いか)
Login
Username or e-mail:

Password:

Remember Me

Lost Password?

Register now!