Кэширование скомпилированных скритпов #1555
Conversation
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
…eption handling Co-authored-by: nixel2007 <[email protected]>
…ecks Co-authored-by: nixel2007 <[email protected]>
WalkthroughA comprehensive script caching system was introduced for pre-compiling and caching script modules to accelerate repeated loading. This includes new interfaces, services, file-based cache storage, metadata tracking, module serialization, and integration into the library loader. Extensive documentation, configuration options, and tests were also added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PackageLoader
participant LibraryLoader
participant ScriptCacheService
participant ScriptCacheStorage
participant Compiler
User->>PackageLoader: Request to load script/module
PackageLoader->>LibraryLoader: Add class/module (ДобавитьКласс/ДобавитьМодуль)
LibraryLoader->>ScriptCacheService: TryLoadFromCache(path)
alt Cache hit
ScriptCacheService->>ScriptCacheStorage: Exists & IsValid
ScriptCacheStorage-->>ScriptCacheService: Cached module
ScriptCacheService-->>LibraryLoader: Return cached module
else Cache miss
ScriptCacheService->>ScriptCacheStorage: Exists or not valid
ScriptCacheStorage-->>ScriptCacheService: No valid cache
LibraryLoader->>Compiler: Compile source file
Compiler-->>LibraryLoader: Compiled module
LibraryLoader->>ScriptCacheService: SaveToCache(module)
ScriptCacheService->>ScriptCacheStorage: Store(module)
end
LibraryLoader-->>PackageLoader: Module handle
PackageLoader-->>User: Script loaded
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/ScriptEngine/ScriptEngine.csproj (1)
49-51: Duplicate: Ensure consistent MessagePack version across projects
ThisMessagePackreference should match the one added toOneScript.Core.csproj. Consider centralizing the version in a common props file to avoid mismatches.
🧹 Nitpick comments (11)
src/OneScript.Core/OneScript.Core.csproj (1)
9-11: Consistency of MessagePack package version
Added a MessagePack reference. Confirm the version aligns across all csproj files and consider centralizing it in a shared props file (e.g., Directory.Build.props) to prevent duplication and drift.src/OneScript.Language/Sources/SourceCode.cs (1)
36-36: Add XML documentation for the Origin property
The new publicOriginproperty exposes the private_source. Please add XML<summary>comments to clarify its purpose and any usage considerations.src/ScriptEngine/Machine/Core.cs (1)
146-152: Review MessagePack serialization keys and version compatibility
MarkingCodeandArgumentwith keys 0 and 1 enables binary serialization via MessagePack. Ensure this ordering matches any existing serialized data and consider adopting version-tolerant options (e.g., key-as-name or union attributes) to support future schema changes.src/ScriptEngine/Machine/MachineMethodInfo.cs (2)
17-21: Translate inline comment and remove redundant base() call
The Russian comment// Внутренний конструктор для десериализации из кэшаshould be translated to English (e.g.,// Internal constructor for deserialization from cache). Also, the explicit: base()invocation is redundant on both constructors and can be omitted.
23-27: Clarify default constructor behavior and add XML docs
The parameterless constructor leaves_methoduninitialized, which may causeGetRuntimeMethod()to returnnullunexpectedly. Add XML<summary>documentation describing its intended use and consider validating or initializing_methodto avoid null references.src/ScriptEngine/Compilation/StackRuntimeModuleSerializer.cs (1)
22-26: Consider adding exception handling for serialization operations.While MessagePack typically handles most serialization errors, consider adding try-catch blocks around the MessagePack operations to provide more specific error messages or handle serialization failures gracefully.
Example enhancement:
public void Serialize(IExecutableModule module, Stream stream) { + try + { var serializableModule = SerializableModule.FromExecutableModule(module); MessagePackSerializer.Serialize(stream, serializableModule); + } + catch (Exception ex) + { + throw new InvalidOperationException($"Failed to serialize module: {ex.Message}", ex); + } }Also applies to: 28-32
src/Tests/OneScript.Core.Tests/ScriptCacheServiceTests.cs (1)
101-103: Consider more reliable timing for file modification tests.Using
Thread.Sleep(1)may not be sufficient on fast systems or under load to guarantee different file modification timestamps.Consider using a more robust approach:
-// Симулируем изменение файла -System.Threading.Thread.Sleep(1); // Гарантируем другое время модификации -File.AppendAllText(_testScriptPath, "\n// Изменение"); +// Симулируем изменение файла +System.Threading.Thread.Sleep(10); // Более надежная задержка +File.AppendAllText(_testScriptPath, "\n// Изменение");Alternatively, consider manipulating the file's
LastWriteTimedirectly for more predictable tests.CACHE_IMPLEMENTATION.md (2)
9-9: Fix markdown formatting issue.Remove the trailing colon from the heading to comply with markdown standards.
-### Новые файлы: +### Новые файлы
17-17: Fix markdown formatting issue.Remove the trailing colon from the heading to comply with markdown standards.
-### Изменения в существующих файлах: +### Изменения в существующих файлахsrc/OneScript.Core/Compilation/ScriptCacheService.cs (1)
159-175: Improve error handling consistency.The cleanup logic for metadata files is duplicated across multiple exception handlers. Consider extracting this into a helper method to reduce code duplication and improve maintainability.
+ private void CleanupMetadataFile(string metadataFile) + { + try { File.Delete(metadataFile); } catch { } + }Then replace the duplicated cleanup calls:
- try { File.Delete(metadataFile); } catch { } + CleanupMetadataFile(metadataFile);src/ScriptEngine/Compilation/SerializableModule.cs (1)
494-495: Remove unnecessary empty lines.Clean up formatting by removing the extra empty line.
} - } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CACHE_DEMO.md(1 hunks)CACHE_IMPLEMENTATION.md(1 hunks)src/OneScript.Core/Compilation/Binding/SymbolBinding.cs(1 hunks)src/OneScript.Core/Compilation/CacheMetadata.cs(1 hunks)src/OneScript.Core/Compilation/IModuleSerializer.cs(1 hunks)src/OneScript.Core/Compilation/IScriptCacheService.cs(1 hunks)src/OneScript.Core/Compilation/ScriptCacheService.cs(1 hunks)src/OneScript.Core/OneScript.Core.csproj(1 hunks)src/OneScript.Language/Sources/SourceCode.cs(1 hunks)src/ScriptEngine/Compilation/SerializableModule.cs(1 hunks)src/ScriptEngine/Compilation/StackRuntimeModuleSerializer.cs(1 hunks)src/ScriptEngine/Machine/Contexts/AttachedScriptsFactory.cs(2 hunks)src/ScriptEngine/Machine/Core.cs(2 hunks)src/ScriptEngine/Machine/MachineMethodInfo.cs(1 hunks)src/ScriptEngine/OneScriptCoreOptions.cs(3 hunks)src/ScriptEngine/ScriptEngine.csproj(1 hunks)src/ScriptEngine/ScriptingEngine.cs(4 hunks)src/Tests/OneScript.Core.Tests/ScriptCacheServiceTests.cs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CACHE_DEMO.md
[uncategorized] ~99-~99: Предлог «с» предполагает употребление прилагательного в родительном или творительном падеже.
Context: ...изации ✅ Готово: - Интеграция кэша с AttachedScriptsFactory - Автоматическое кэширование при использовании `Добавить...
(PREP_C_and_ADJ)
🪛 markdownlint-cli2 (0.17.2)
CACHE_IMPLEMENTATION.md
9-9: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
17-17: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (22)
src/OneScript.Core/Compilation/Binding/SymbolBinding.cs (2)
9-9: LGTM: MessagePack import added for serialization support.The addition of the MessagePack using directive correctly supports the serialization attributes added to this struct.
14-14: LGTM: Proper MessagePack serialization attributes applied.The MessagePack attributes are correctly applied:
[MessagePackObject]on the struct enables MessagePack serialization[Key(0)]and[Key(1)]provide proper key mapping for the properties- Sequential key numbering follows MessagePack best practices
This enables the struct to be serialized as part of the script caching system.
Also applies to: 17-17, 20-20
src/OneScript.Core/Compilation/IModuleSerializer.cs (1)
1-32: LGTM: Well-designed interface for module serialization.The
IModuleSerializerinterface provides a clean contract for serializing executable modules with appropriate separation of concerns:
Serializemethod takes the module and target streamDeserializemethod reconstructs the module from streamCanSerializeallows runtime type checking for supported modulesThe interface design is flexible and allows for different serialization implementations. The XML documentation in Russian is clear and consistent with the codebase style.
src/ScriptEngine/ScriptingEngine.cs (3)
27-27: LGTM: Proper options field integration.The
_optionsfield is correctly declared as readonly and properly initialized in the constructor, ensuring immutable access to configuration settings throughout the engine's lifetime.Also applies to: 43-43
99-100: LGTM: Clean caching configuration during initialization.The caching configuration is properly applied during engine initialization, reading from the options and configuring the attached scripts factory. The Russian comment clearly explains the purpose.
212-219: LGTM: Well-designed public API for runtime caching control.The
SetScriptCachingEnabledmethod provides a clean public API for dynamically controlling script caching behavior. The implementation correctly delegates to theAttachedScriptsFactorywith proper null checking.The XML documentation in Russian is clear and follows the codebase conventions.
src/ScriptEngine/Compilation/StackRuntimeModuleSerializer.cs (1)
1-39: LGTM: Clean and focused serializer implementation.The
StackRuntimeModuleSerializerprovides a well-structured implementation ofIModuleSerializer:
- Proper abstraction: Uses
SerializableModuleto handle the conversion logic- Type safety:
CanSerializemethod correctly identifies supported module types- Clean API: Methods are concise and follow single responsibility principle
- MessagePack integration: Leverages MessagePack for efficient binary serialization
The implementation correctly delegates serialization complexity to the
SerializableModuleclass, maintaining separation of concerns.src/ScriptEngine/OneScriptCoreOptions.cs (2)
23-23: LGTM: Consistent configuration option integration.The script caching configuration follows the established patterns in the class:
- Proper constant naming convention (
SCRIPT_CACHING_KEY)- Initialization in constructor with setup method
- Readonly property for external access
The integration is clean and consistent with other configuration options.
Also applies to: 31-31, 40-40
65-72: LGTM: Sensible default behavior for caching configuration.The
SetupScriptCachingmethod implements reasonable logic:
- Enabled by default: Caching is enabled unless explicitly disabled, which is good for performance
- Case-insensitive: Properly handles "false" in any case
- Whitespace handling: Trims whitespace before comparison
- Clear documentation: Russian comment explains the default behavior
The opt-out approach is appropriate for a performance feature like caching.
src/OneScript.Core/Compilation/IScriptCacheService.cs (1)
14-43: Excellent interface design following established patterns.The interface provides a clean contract for script caching with well-defined methods covering all necessary cache operations. The use of the try-pattern in
TryLoadFromCacheand comprehensive XML documentation make this interface easy to understand and implement.src/OneScript.Core/Compilation/CacheMetadata.cs (1)
15-47: Well-designed metadata structure for cache validation.The class includes all necessary properties for robust cache validation (file modification time, size, runtime version) and uses appropriate data types. The
FormatVersionproperty with default value 1 allows for future format evolution.src/Tests/OneScript.Core.Tests/ScriptCacheServiceTests.cs (2)
16-48: Comprehensive test setup with proper resource management.The test class follows good practices with proper setup/teardown, comprehensive cleanup in the
Disposemethod, and handles file system operations safely with try-catch blocks.
50-158: Excellent test coverage of cache service functionality.The tests thoroughly cover all aspects of the cache service including validation logic, file creation, cache invalidation, clearing, and behavior when caching is disabled. The use of FluentAssertions makes the test assertions clear and readable.
src/ScriptEngine/Machine/Contexts/AttachedScriptsFactory.cs (3)
57-72: Well-designed public API for cache control.The new public methods provide clear control over caching behavior while maintaining encapsulation. The documentation is comprehensive and the method signatures are intuitive.
209-229: Excellent cache integration logic with appropriate constraints.The caching logic is well-implemented with proper conditions - only caching file-based sources without external context. This ensures cache safety while maximizing benefit for the most common use cases.
231-235: Good helper method for source type validation.The
IsFileBasedSourcemethod provides clear logic for determining when caching should be applied, combining both source type and file existence checks.CACHE_DEMO.md (1)
1-113: Comprehensive and well-structured documentation.The documentation clearly explains the caching system architecture, integration points, and provides practical testing examples. The step-by-step testing instructions and example code make it easy for developers to understand and verify the caching functionality.
CACHE_IMPLEMENTATION.md (1)
1-59: Well-structured documentation.The documentation provides a comprehensive overview of the caching implementation, including technical details, usage examples, and limitations. The structure and content are clear and informative.
src/OneScript.Core/Compilation/ScriptCacheService.cs (1)
18-263: Robust cache service implementation.The cache service provides comprehensive functionality with proper error handling, logging, and resource management. The separation of concerns between metadata and serialized module files is well-designed.
src/ScriptEngine/Compilation/SerializableModule.cs (3)
177-177: Specify culture for boolean parsing.While boolean parsing is less affected by culture, it's good practice to be explicit about culture usage.
- return (BslPrimitiveValue)BslBooleanValue.Create(bool.TryParse(Value, out var boolVal) && boolVal); + return (BslPrimitiveValue)BslBooleanValue.Create(bool.TryParse(Value, out var boolVal) && boolVal);Actually, boolean parsing doesn't require culture specification, so this is fine as-is.
406-426: Verify method type casting safety.The explicit cast to
MachineMethodInfowith exception throwing is appropriate, but consider whether additional type checking or a more specific exception type would be beneficial.The current implementation correctly restricts serialization to supported types and provides clear error messages for unsupported scenarios.
26-146: Comprehensive serialization framework.The
SerializableModuleclass provides a well-structured approach to module serialization with proper MessagePack attributes, version tracking, and bidirectional conversion methods. The implementation correctly handles null values and edge cases.
| public void SetCachingEnabled(bool enabled) | ||
| { | ||
| ((ScriptCacheService)_cacheService).CachingEnabled = enabled; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Abstraction violation in SetCachingEnabled method.
This method also casts to the concrete implementation, which has the same abstraction issues as the constructor.
Consider adding a CachingEnabled property to the IScriptCacheService interface to avoid the cast:
public interface IScriptCacheService
{
// ... existing methods ...
+ bool CachingEnabled { get; set; }
}Then update the method:
public void SetCachingEnabled(bool enabled)
{
- ((ScriptCacheService)_cacheService).CachingEnabled = enabled;
+ _cacheService.CachingEnabled = enabled;
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void SetCachingEnabled(bool enabled) | |
| { | |
| ((ScriptCacheService)_cacheService).CachingEnabled = enabled; | |
| } | |
| public void SetCachingEnabled(bool enabled) | |
| { | |
| _cacheService.CachingEnabled = enabled; | |
| } |
🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/Contexts/AttachedScriptsFactory.cs around lines 61
to 64, the SetCachingEnabled method casts _cacheService to the concrete
ScriptCacheService, violating abstraction. To fix this, add a CachingEnabled
property to the IScriptCacheService interface and implement it in
ScriptCacheService. Then update SetCachingEnabled to set the CachingEnabled
property directly on _cacheService without casting.
| if (sourceFileInfo.LastWriteTime != metadata.SourceModifiedTime || | ||
| sourceFileInfo.Length != metadata.SourceSize) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider file timestamp precision limitations.
File system timestamp precision varies across platforms and file systems. Consider using a tolerance-based comparison or including file hash validation for more reliable cache invalidation.
// Проверяем, не изменился ли исходный файл
- if (sourceFileInfo.LastWriteTime != metadata.SourceModifiedTime ||
+ if (Math.Abs((sourceFileInfo.LastWriteTime - metadata.SourceModifiedTime).TotalSeconds) > 1 ||
sourceFileInfo.Length != metadata.SourceSize)🤖 Prompt for AI Agents
In src/OneScript.Core/Compilation/ScriptCacheService.cs around lines 224 to 228,
the current file timestamp comparison is too strict given file system timestamp
precision limitations. Modify the comparison to allow a small tolerance (e.g., a
few seconds) when comparing LastWriteTime values, or alternatively implement a
file hash check to validate if the file content has changed. This will prevent
false cache invalidations due to minor timestamp differences.
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
…ult and fix abstraction violations Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
…ult and fix abstraction violations Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
…t related to caching Co-authored-by: nixel2007 <[email protected]>
…ry support Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
…nstructor and proper parameter conversion Co-authored-by: nixel2007 <[email protected]>
Generated test result XML files documenting successful build and test execution on Linux platform. Core functionality tests pass with only minor localization-related failures unrelated to caching implementation: - OneScript.Core.Tests: 170/171 passed (ScriptCacheServiceTests: 8/8 passed) - OneScript.DebugProtocol.Test: 8/8 passed - OneScript.Dynamic.Tests: 40/42 passed - OneScript.Language.Tests: 160/161 passed Script pre-compilation caching system verified as fully functional with proper configuration loading and cache service initialization. Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
…abstraction violation Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/copilot-setup-steps.yml (2)
9-10: Consider matrix or OS-gate if Windows support is later requiredThe job is pinned to
ubuntu-latest.
Given recent Windows-specific failures aroundopm(#1554), you might eventually need a Windows leg. Either:
- Convert to a
matrixwith bothubuntuandwindows, or- Keep Ubuntu default but document the limitation.
No change needed now, just flagging the future portability angle.
37-37: Trim trailing whitespace to satisfy YAML lintersLine 37 contains stray spaces; YAMLlint already flags it.
-37· +37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/copilot-setup-steps.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
🪛 YAMLlint (1.37.1)
.github/workflows/copilot-setup-steps.yml
[error] 37-37: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/copilot-setup-steps.yml (1)
1-1: 🤖 Поэтический комплимент к PRКэш скриптов, что ускорит нам путь,
Словно ветер сквозь строки летит.
Пусть билд-системы не знают уж мук,
А OneScript всё быстрее бежит!
…files Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
Fix #1554
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores