VM: имена свойств и методов в отдельный список #1642
Conversation
📝 WalkthroughWalkthroughAdds identifier interning: compiler now emits identifier indices instead of embedding constant strings; runtime resolves names via module.Identifiers; ModuleDumpWriter prints a new ".identifiers" section. Many collection emptiness checks changed from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Generator as CodeGenerator
participant Module as StackRuntimeModule
participant Runtime as MachineInstance
participant Dumper as ModuleDumpWriter
rect rgb(230,245,255)
Note over Generator,Module: Compile-time identifier interning
Generator->>Module: GetIdentNumber("foo")
activate Module
Module-->>Generator: identIndex (adds "foo" if missing)
deactivate Module
Generator->>Module: Emit command referencing identIndex
end
rect rgb(245,255,230)
Note over Runtime,Module: Runtime resolution
Runtime->>Module: fetch Identifiers[identIndex]
Module-->>Runtime: "foo"
Runtime->>Runtime: resolve property/method/type by name
end
rect rgb(255,245,230)
Note over Dumper,Module: Dumping module
Dumper->>Module: Read Constants, then Identifiers
Module-->>Dumper: list of identifier strings
Dumper->>Dumper: emit ".identifiers" section
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.cs📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Files:
🧠 Learnings (2)📚 Learning: 2024-07-25T09:44:11.960ZApplied to files:
📚 Learning: 2025-12-18T16:13:05.448ZApplied to files:
🧬 Code graph analysis (2)src/OneScript.Core/Contexts/ClassBuilder.cs (2)
src/ScriptEngine/Machine/MachineInstance.cs (2)
🔇 Additional comments (8)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (1)
1324-1336: Consider case-insensitive identifier matching.The
GetIdentNumberhelper usesList<string>.IndexOfwhich performs case-sensitive comparison. However, OneScript identifiers are typically case-insensitive. If the same property/method is referenced with different casing (e.g.,MyPropvsmyProp), they would be stored as separate entries.Verify whether case-insensitive matching is required here. If so, consider using
FindIndexwith a case-insensitive comparer:🔎 Suggested fix for case-insensitive matching
private int GetIdentNumber(string ident) { - - var idx = _module.Identifiers.IndexOf(ident); + var idx = _module.Identifiers.FindIndex(x => + string.Equals(x, ident, StringComparison.OrdinalIgnoreCase)); if (idx < 0) { idx = _module.Identifiers.Count; _module.Identifiers.Add(ident); } return idx; }Also, minor formatting nit: there's an extra blank line at line 1326 inside the method body.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/ScriptEngine/Compiler/ModuleDumpWriter.cssrc/ScriptEngine/Compiler/StackMachineCodeGenerator.cssrc/ScriptEngine/Machine/MachineInstance.cssrc/ScriptEngine/Machine/StackRuntimeModule.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/ScriptEngine/Machine/MachineInstance.cssrc/ScriptEngine/Compiler/StackMachineCodeGenerator.cssrc/ScriptEngine/Compiler/ModuleDumpWriter.cssrc/ScriptEngine/Machine/StackRuntimeModule.cs
🧠 Learnings (1)
📚 Learning: 2025-12-18T16:13:05.448Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1636
File: src/OneScript.StandardLibrary/StringOperations.cs:155-157
Timestamp: 2025-12-18T16:13:05.448Z
Learning: Guideline: In OneScript, when a method parameter has a concrete type (e.g., string) and is called from scripts with omitted arguments, the runtime passes an empty string "" rather than null. Direct (non-script) calls to the method may still pass null, so implement defensive null checks for these parameters in public methods that can be called from scripts. Treat empty string as a legitimate value for script calls, and explicitly handle null for direct calls (e.g., fail fast, throw ArgumentNullException, or normalize to "" where appropriate). This should apply to all C# methods that may be invoked from OneScript with optional string parameters across the codebase.
Applied to files:
src/ScriptEngine/Machine/MachineInstance.cssrc/ScriptEngine/Compiler/StackMachineCodeGenerator.cssrc/ScriptEngine/Compiler/ModuleDumpWriter.cssrc/ScriptEngine/Machine/StackRuntimeModule.cs
🧬 Code graph analysis (1)
src/ScriptEngine/Machine/MachineInstance.cs (6)
src/OneScript.Core/Values/BslDateValue.cs (1)
ToString(61-64)src/OneScript.Core/Contexts/Variable.cs (2)
ToString(150-153)ToString(163-166)src/ScriptEngine/Machine/Contexts/UserScriptContextInstance.cs (1)
ToString(110-113)src/OneScript.Core/Values/BslBooleanValue.cs (1)
ToString(53-56)src/OneScript.Core/Values/BslValue.cs (1)
ToString(25-28)src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs (1)
ToString(41-44)
🔇 Additional comments (8)
src/ScriptEngine/Compiler/ModuleDumpWriter.cs (1)
132-138: LGTM!The new
.identifierssection is well-structured and consistent with the existing dump format for constants. The output formatting aligns with the established style of the dump writer.src/ScriptEngine/Machine/MachineInstance.cs (3)
679-684: LGTM!Minor formatting adjustment with no functional impact.
1014-1014: LGTM!The identifier-based resolution correctly uses the new
Identifierslist for property and method name lookup. This eliminates the unnecessary string-to-IValue-to-string conversions, achieving the PR's optimization goal.Also applies to: 1051-1051
1194-1194: Verify the simplified ToString() call for type name resolution.The change from
PopRawBslValue().ToString(_process)to_operationStack.Pop().ToString()relies on code generation always pushing aBslStringValue. While this should work for the staticnewcase (as indicated by the comment), consider whether:
- The value could be an
IVariablewrapper requiring dereferencing viaRawValue()- Any edge cases exist where
ToString(_process)would produce different results thanToString()Based on the relevant code snippets showing
BslValue.ToString(IBslProcess)defaults toToString(), this appears safe for string values, but worth confirming there are no scenarios where the stack contains a reference type here.src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (3)
492-492: LGTM!The explicit count check (
node.Children.Count != 0) is clearer and consistent with the pattern used inVisitReturnNodeat line 480.
731-736: LGTM!The method resolution now correctly uses identifier-based lookup via
GetIdentNumber, which stores method names in the dedicatedIdentifierslist instead of the constants pool.
739-742: LGTM!Property resolution now uses the identifier table, consistent with the method resolution changes.
src/ScriptEngine/Machine/StackRuntimeModule.cs (1)
29-34: TheCodeproperty at line 57 is not part of theIExecutableModuleinterface contract—the interface does not define aCodemember. Therefore, typing it asList<Command>does not create interface compatibility issues. The property is implementation-specific toStackRuntimeModuleand can safely use the concreteList<T>type.The use of
List<T>for the internal propertiesIdentifiers,VariableRefs, andMethodRefsis appropriate since these are internal members and enables efficient operations likeIndexOf.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ScriptEngine/Machine/MachineInstance.cs (1)
27-27: Remove unused import directive.The
using System.Dynamic;directive appears unused in this file—there are no references to dynamic types or DLR infrastructure.Proposed cleanup
-using System.Dynamic;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/OneScript.Core/Contexts/ClassBuilder.cssrc/OneScript.Language/SyntaxAnalysis/BslSyntaxWalker.cssrc/OneScript.Language/SyntaxAnalysis/ConditionalDirectiveHandler.cssrc/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cssrc/OneScript.Native/Compiler/MethodCompiler.cssrc/OneScript.Native/Compiler/StatementBlocksWriter.cssrc/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cssrc/OneScript.StandardLibrary/CustomLineFeedStreamReader.cssrc/OneScript.StandardLibrary/Tasks/BackgroundTasksManager.cssrc/ScriptEngine/Compiler/StackMachineCodeGenerator.cssrc/ScriptEngine/Machine/MachineInstance.cssrc/oscript/Web/Multipart/BinaryStreamStack.cs
✅ Files skipped from review due to trivial changes (3)
- src/OneScript.Language/SyntaxAnalysis/BslSyntaxWalker.cs
- src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs
- src/OneScript.Language/SyntaxAnalysis/ConditionalDirectiveHandler.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/OneScript.StandardLibrary/CustomLineFeedStreamReader.cssrc/OneScript.Core/Contexts/ClassBuilder.cssrc/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cssrc/OneScript.Native/Compiler/MethodCompiler.cssrc/OneScript.StandardLibrary/Tasks/BackgroundTasksManager.cssrc/oscript/Web/Multipart/BinaryStreamStack.cssrc/ScriptEngine/Compiler/StackMachineCodeGenerator.cssrc/ScriptEngine/Machine/MachineInstance.cssrc/OneScript.Native/Compiler/StatementBlocksWriter.cs
🧠 Learnings (3)
📚 Learning: 2025-12-18T16:13:05.448Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1636
File: src/OneScript.StandardLibrary/StringOperations.cs:155-157
Timestamp: 2025-12-18T16:13:05.448Z
Learning: Guideline: In OneScript, when a method parameter has a concrete type (e.g., string) and is called from scripts with omitted arguments, the runtime passes an empty string "" rather than null. Direct (non-script) calls to the method may still pass null, so implement defensive null checks for these parameters in public methods that can be called from scripts. Treat empty string as a legitimate value for script calls, and explicitly handle null for direct calls (e.g., fail fast, throw ArgumentNullException, or normalize to "" where appropriate). This should apply to all C# methods that may be invoked from OneScript with optional string parameters across the codebase.
Applied to files:
src/OneScript.StandardLibrary/CustomLineFeedStreamReader.cssrc/OneScript.Core/Contexts/ClassBuilder.cssrc/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cssrc/OneScript.Native/Compiler/MethodCompiler.cssrc/OneScript.StandardLibrary/Tasks/BackgroundTasksManager.cssrc/oscript/Web/Multipart/BinaryStreamStack.cssrc/ScriptEngine/Compiler/StackMachineCodeGenerator.cssrc/ScriptEngine/Machine/MachineInstance.cssrc/OneScript.Native/Compiler/StatementBlocksWriter.cs
📚 Learning: 2024-07-25T09:44:11.960Z
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 1426
File: src/OneScript.Web.Server/HeaderDictionaryWrapper.cs:43-50
Timestamp: 2024-07-25T09:44:11.960Z
Learning: The `CanWrite` attribute in the `ContextProperty` attribute is used for different purposes and is not related to the presence of a setter in the property.
Applied to files:
src/OneScript.Core/Contexts/ClassBuilder.cs
📚 Learning: 2025-09-04T11:15:14.305Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1578
File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64
Timestamp: 2025-09-04T11:15:14.305Z
Learning: In OneScript, all objects that implement `IValue` are descendants of `BslValue`, making casting from `IValue` to `BslValue` type-safe.
Applied to files:
src/ScriptEngine/Machine/MachineInstance.cs
🧬 Code graph analysis (3)
src/OneScript.Core/Contexts/ClassBuilder.cs (3)
src/OneScript.Core/Contexts/ContextPropertyInfo.cs (2)
GetCustomAttributes(34-37)GetCustomAttributes(39-42)src/OneScript.Core/Contexts/ContextMethodInfo.cs (2)
GetCustomAttributes(48-51)GetCustomAttributes(53-56)src/OneScript.Core/Contexts/AnnotationHolder.cs (2)
GetCustomAttributes(23-26)GetCustomAttributes(28-31)
src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (1)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (1)
Count(135-138)
src/OneScript.StandardLibrary/Tasks/BackgroundTasksManager.cs (8)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (2)
Count(27-27)Count(29-29)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (1)
Count(122-122)src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeRow.cs (2)
Count(38-41)Count(43-43)src/ScriptEngine/Machine/Contexts/EnumerationContext.cs (1)
Count(113-116)src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeColumnCollection.cs (1)
Count(93-93)src/OneScript.StandardLibrary/Collections/StructureImpl.cs (1)
Count(206-206)src/OneScript.StandardLibrary/Collections/FixedStructureImpl.cs (1)
Count(136-136)src/ScriptEngine/Machine/Contexts/AutoCollectionContext.cs (2)
Count(28-28)Count(30-30)
🔇 Additional comments (23)
src/OneScript.StandardLibrary/CustomLineFeedStreamReader.cs (1)
59-59: LGTM! Consistency improvement.The change from
> 0to!= 0is functionally equivalent and aligns with the project-wide pattern for count checks.src/OneScript.StandardLibrary/Tasks/BackgroundTasksManager.cs (1)
123-123: LGTM! Micro-optimization aligns with PR objectives.The change from
Any()toCount != 0avoids LINQ enumeration overhead while maintaining identical semantics. SincefailedTasksis aList<T>, theCountproperty access is O(1).src/OneScript.Native/Compiler/StatementBlocksWriter.cs (1)
21-21: LGTM! Micro-optimization applied consistently.The change from
Count > 0toCount != 0is functionally equivalent and aligns with the micro-optimization pattern applied throughout this PR.src/OneScript.Language/SyntaxAnalysis/DefaultBslParser.cs (2)
126-126: LGTM: Collection emptiness check normalized.The change from
!Any()toCount == 0is semantically equivalent and aligns with the broader repository pattern of normalizing collection checks.
1653-1653: LGTM: Non-emptiness check normalized.The change from
Count > 0toCount != 0is semantically equivalent for the stack's non-negative count and follows the repository-wide pattern.src/OneScript.Native/Compiler/MethodCompiler.cs (4)
716-733: LGTM: Stack emptiness checks are correct.The changes from
Count > 0toCount != 0are semantically equivalent and align with the PR's micro-optimization objectives.
1147-1153: LGTM: Children count check is correct.The emptiness check for the argument's children collection is correct and consistent with the PR's optimization pattern.
1416-1419: LGTM: Argument children check is correct.The collection emptiness check is correct, properly distinguishing between passed and omitted arguments.
1525-1528: LGTM: Constructor argument check is correct.The children count check correctly handles both provided and omitted constructor arguments.
src/oscript/Web/Multipart/BinaryStreamStack.cs (4)
91-94: LGTM: Efficient collection emptiness check.Replacing
Any()withCount != 0eliminates LINQ overhead and is more idiomatic forStack<T>.
185-185: LGTM: Consistent optimization.The loop condition correctly uses
Count != 0, aligning with theNextStream()implementation and avoiding LINQ overhead.
231-231: LGTM: Consistent optimization.The loop condition correctly uses
Count != 0, matching the pattern in theRead(byte[], int, int)overload.
399-399: LGTM: Consistent optimization.The ternary expression correctly uses
Count != 0, aligning with the overall refactoring pattern in this file.src/ScriptEngine/Machine/MachineInstance.cs (6)
1010-1021: LGTM: Identifier-based property resolution.The change from constant-based to identifier-based property name resolution is correct and aligns with the PR's optimization goal.
1045-1098: LGTM: Identifier-based method name resolution.The change from constant-based to identifier-based method name resolution is correct and consistent with the broader refactoring.
1132-1150: LGTM: More idiomatic count check.The change from
Count > 0toCount != 0is more idiomatic for collection emptiness checks.
1370-1406: LGTM: More efficient collection check.The change from
!module.Methods.Any()tomodule.Methods.Count == 0is more efficient—it avoids allocating an enumerator.
2370-2407: LGTM: Well-refactored type instantiation logic.The extraction of
CreateInstancecentralizes type lookup and factory activation logic, making it reusable for bothNewFuncandNewInstance.
1184-1199: Stack protocol for NewInstance opcode is correctly implemented.The compiler's
MakeNewObjectStaticflow always emitsArgNumbeforeNewInstance:
- Via
PushCallArgumentsfor cases with arguments- Via explicit
AddCommand(OperationCode.ArgNum, 0)for no-argument casesThe refactored
NewInstancemethod correctly consumes this protocol: it pops the argument count from the stack (Line 1186) and receives the identifier index as theargparameter (Line 1196).src/ScriptEngine/Compiler/StackMachineCodeGenerator.cs (4)
720-736: LGTM: Identifier-based method resolution.The change to emit identifier indices for
ResolveMethodFuncandResolveMethodProcis correct and aligns with the runtime changes in MachineInstance.cs.
738-741: LGTM: Identifier-based property resolution.The change to emit identifier indices for
ResolvePropis correct and consistent with the broader refactoring.
1063-1076: LGTM: Stack protocol matches runtime changes.The
MakeNewObjectStaticrefactoring correctly emits the stack protocol expected byMachineInstance.NewInstance:
PushCallArgumentspushes arguments and thenArgNum(count on stack)NewInstanceopcode takes identifier index as argument and pops count from stack
1321-1330: LGTM: Identifier interning implementation.The
GetIdentNumbermethod correctly implements identifier interning by maintaining a deduplicated list in_module.Identifiers. The implementation follows the same pattern asGetConstNumber.
|
Замечание про ClassBuilder.MarkedAsContextProperty - будто действительно баг? |
Надо уточнить места вызовов, чтобы клиентский код получил тот же ответ. А так да, похоже на баг |
|
@Mr-Rm а есть цифры, насколько стало быстрее? Просто любопытно |
Вызов только из одного места, по умолчанию |
|
По цифрам: по сравнению с rc-10, с изменениями последних четырех PR время запуска oneunit сократилось с 2.1 до 1.8 секунд, winow - c 2.6 до 2.3 (оба без параметров). Но основное время съедает загрузка библиотек: их насчитал 44 с 272 классами в oneunit и 40 с 303 классами в winow. Тут уже нужно ускорять работу package-loader'а. |
2 participants
Поскольку имена свойств, методов, полей структур являются по определению корректными идентификаторами, их можно хранить в отдельном от произвольных констант списке в виде строк, а не преобразовывать в IValue и обратно.
То же относится и именам типов и классов.
Summary by CodeRabbit
New Features
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.