Grapherをキャッシュ可能に #24
Grapherをキャッシュ可能に #24
Conversation
|
Warning Rate limit exceeded@koriym has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 33 minutes and 43 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a new dependency on Doctrine Cache and refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant Composer as composer.json
participant AppModule as AppModule.php
participant Dii as Dii.php
participant Cache as Doctrine Cache
participant Test as DiiTest.php
Composer->>AppModule: Add doctrine/cache dependency
AppModule->>Dii: Import and bind SiteController
Dii->>Cache: Use Doctrine Cache
Dii->>Test: Modify setContext method
Test->>Dii: Initialize context in setUp method
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- composer.json (1 hunks)
- demo/src-module/Module/AppModule.php (2 hunks)
- src/Dii.php (4 hunks)
- tests/Fake/Module/AppModule.php (1 hunks)
- tests/UnitTest/DiiTest.php (1 hunks)
Additional comments not posted (9)
demo/src-module/Module/AppModule.php (2)
8-8: The import ofSiteControlleris correctly placed and necessary for the binding within theconfiguremethod.
23-23: The binding ofSiteControlleris correctly implemented and aligns with the PR's objective to enhance module configuration.tests/Fake/Module/AppModule.php (1)
22-22: The binding ofFakeSiteControlleris correctly implemented for testing purposes and aligns with the PR's objective to enhance the testing setup.composer.json (2)
21-21: The update to theray/diversion is appropriate and likely necessary for compatibility with the new features introduced in the PR.
22-22: The addition ofdoctrine/cacheis essential for the new caching capabilities introduced in the PR and is correctly added to the dependencies.tests/UnitTest/DiiTest.php (1)
20-20: The addition ofDii::setContext(App::class)in thesetUpmethod is correctly implemented to align with the new requirement that setting the context is mandatory before running tests.src/Dii.php (3)
8-9: Imports forDoctrine\Common\Cache\ArrayCacheandDoctrine\Common\Cache\Cachehave been added.These imports are necessary for the new caching functionality introduced in the
setContextmethod.
37-38: Initialization of a private static variable$grapherof typeGrapher.This change aligns with the refactoring to use
Grapherfor dependency injection and object creation.
75-76: Use ofGrapherinstance to create new instances if the class is injectable.This change is part of the refactoring to utilize
Grapherfor object instantiation, improving the flexibility and maintainability of the component creation process.
| public static function setContext(string $contextClass, ?Cache $cache = null, ?string $tmpDir = null): void | ||
| { | ||
| if (! class_exists($context)) { | ||
| throw new LogicException("Unloadable: {$context}"); | ||
| $cache = $cache ?? new ArrayCache(); | ||
| $tmpDir = $tmpDir ?? dirname((new ReflectionClass($contextClass))->getFileName()) . '/tmp'; | ||
| if (! class_exists($contextClass)) { | ||
| throw new LogicException("Not found context class: {$contextClass}"); | ||
| } | ||
|
|
||
| assert(class_exists($context)); | ||
| self::$context = $context; | ||
| self::createModule(); | ||
| /** @var ?Grapher $cachedModule */ | ||
| self::$grapher = $cache->fetch($contextClass); | ||
| if (! self::$grapher instanceof Grapher) { | ||
| $module = (new $contextClass())(); | ||
| self::$grapher = new Grapher($module, $tmpDir); | ||
| } |
There was a problem hiding this comment.
Significant changes to the setContext method to support caching.
The method now accepts optional parameters for cache and temporary directory, and includes logic to handle caching of the Grapher instance. Consider adding detailed documentation about the behavior when cache is not provided or is empty, and the implications of using a default ArrayCache.
+ /**
+ * Sets the application context and initializes caching.
+ * @param string $contextClass The context class name.
+ * @param Cache|null $cache Optional cache backend to use.
+ * @param string|null $tmpDir Optional temporary directory for cache files.
+ * @throws LogicException If the context class is not found.
+ */Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| public static function setContext(string $contextClass, ?Cache $cache = null, ?string $tmpDir = null): void | |
| { | |
| if (! class_exists($context)) { | |
| throw new LogicException("Unloadable: {$context}"); | |
| $cache = $cache ?? new ArrayCache(); | |
| $tmpDir = $tmpDir ?? dirname((new ReflectionClass($contextClass))->getFileName()) . '/tmp'; | |
| if (! class_exists($contextClass)) { | |
| throw new LogicException("Not found context class: {$contextClass}"); | |
| } | |
| assert(class_exists($context)); | |
| self::$context = $context; | |
| self::createModule(); | |
| /** @var ?Grapher $cachedModule */ | |
| self::$grapher = $cache->fetch($contextClass); | |
| if (! self::$grapher instanceof Grapher) { | |
| $module = (new $contextClass())(); | |
| self::$grapher = new Grapher($module, $tmpDir); | |
| } | |
| /** | |
| * Sets the application context and initializes caching. | |
| * @param string $contextClass The context class name. | |
| * @param Cache|null $cache Optional cache backend to use. | |
| * @param string|null $tmpDir Optional temporary directory for cache files. | |
| * @throws LogicException If the context class is not found. | |
| */ | |
| public static function setContext(string $contextClass, ?Cache $cache = null, ?string $tmpDir = null): void | |
| { | |
| $cache = $cache ?? new ArrayCache(); | |
| $tmpDir = $tmpDir ?? dirname((new ReflectionClass($contextClass))->getFileName()) . '/tmp'; | |
| if (! class_exists($contextClass)) { | |
| throw new LogicException("Not found context class: {$contextClass}"); | |
| } | |
| /** @var ?Grapher $cachedModule */ | |
| self::$grapher = $cache->fetch($contextClass); | |
| if (! self::$grapher instanceof Grapher) { | |
| $module = (new $contextClass())(); | |
| self::$grapher = new Grapher($module, $tmpDir); | |
| } |
The import statements within Dii.php have been rearranged. The use statement for Doctrine's "AnnotationRegistry" has been repositioned higher in the list to better reflect its relevance within the code.
|
doctrine/cacheが非推奨になっているので一旦draftにします。 |
概要
コンテキストをセットするときにdoctrine/cacheオブジェクトを渡すとキャッシュが行われます。
効果
以下のコストが節約されます。
グラファーの生成は全てのDI/AOP束縛をループして対象を検査し、シンタックスツリーを生成しPHPコードを出力する高コストなものです。そのコストをランタイムで0にします。
バージョン
Dii::setContext()でのコンテキストの指定が必須になります。指定していない時のディフォルトコンテキストはありません。以上の二点で後方互換性が破壊されるのでバージョンは
0.4になります。パフォーマンス以外の問題点
v0.3で束縛されていないクラスに対してランタイムでの暗黙的束縛が行われていましたが、この暗黙的束縛はAOPのコンパイルが行われておらず、同じ対象コードでも明示的束縛をしたときと振る舞いが変わっていました。例)
明示的束縛の強制によるメリット
アンターゲット束縛の明示的な束縛には、以下のメリットもあると考えました。
デメリットは以下のようなものが検討されました。
対策
束縛のコードが必要な対策として、フォルダをスキャンして束縛するモジュールの用意が考えられます。（必要なら別PRで）
パフォーマンスコストの性質
現状では束縛が増えるのに応じて、実行時のパフォーマンスは悪化しますがこのPRによる改良ではコストを初期化時に寄せる事ができます。
設計指針
メンテナンスしやしくパフォーマンスの良いコードを実現するために、オブジェクトの状態を安定的にしランタイムでより少ないコード実行を行うような指針を持っています。このPRはその指針に基づいたものです。
Summary by CodeRabbit
New Features
doctrine/cache.Enhancements
SiteControllerandFakeSiteController.Refactor
Diiclass to enhance caching and object instantiation processes.Tests