Refactor AST code in ShaderLab for performance #2486
Conversation
Sway007
added
Rendering
WalkthroughThe pull request involves modifications across multiple files in the Changes
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2486 +/- ##
========================================
Coverage 68.44% 68.44%
========================================
Files 925 925
Lines 96170 96196 +26
Branches 8155 8127 -28
========================================
+ Hits 65821 65841 +20
- Misses 30094 30100 +6
Partials 255 255
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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 (4)
tests/vitest.config.ts (1)
22-23: Parameterize headless mode for flexible test runs.Currently, the line is commented out, disabling headless mode. For CI environments, you might still want headless mode enabled. Consider adding an environment-based toggle, so local development can run with a visible browser, while CI remains headless when needed.
Here is an illustrative diff (optional):
test: { browser: { provider: "playwright", enabled: true, name: "chromium", providerOptions: { launch: { args: ["--use-gl=egl", "--ignore-gpu-blocklist", "--use-gl=angle"] } } - // headless: true + headless: process.env.CI ? true : false } }packages/shader-lab/src/parser/AST.ts (3)
529-553: Simplify repeated logic in parameter list handling.
The logic for pushing parameter info is duplicated between the single and multiple param branches. Consider factoring out repeated code to improve readability and maintainability.
727-758: Refactor repeated parameter list code.
Similar to the function parameter handling, there is repeated code for single vs. multiple arguments. Consolidating might make it more maintainable.
1039-1061: Multiple expansions inStructDeclarationList.
Collection logic is correct, though there is repeated iteration over lists. You can merge or unify the steps to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/shader-lab/src/lexer/Utils.ts(1 hunks)packages/shader-lab/src/parser/AST.ts(17 hunks)packages/shader-lab/src/preprocessor/PpScanner.ts(1 hunks)tests/vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/shader-lab/src/preprocessor/PpScanner.ts
- packages/shader-lab/src/lexer/Utils.ts
🔇 Additional comments (20)
packages/shader-lab/src/parser/AST.ts (20)
209-222: Consider verifying the coherence of qualifier usage.
The newqualifierListproperty offers flexibility, but it's unclear if multiple qualifiers might contradict each other or if some require special handling. Ensure no conflicts or redundant qualifiers degrade performance or complexity.
260-266: Check assumptions when casting the first child to a token.
This assumes the first child is always a token. If parsing logic changes in the future, it might cause runtime errors.
286-301: ValidateisCustomlogic.
SettingisCustomtotypeof this.type === "string"might incorrectly classify built-in types also represented as strings. If there's a chance that built-in types are assigned as strings, consider a more robust distinguishing mechanism.
307-310: Handle potential invalid or negative array sizes.
Currently, lines 307-310 setsizeto the integer constant but do not check if it's non-negative or within sensible limits.
317-321: Consider edge cases for division operator.
Division by zero would silently fail or produce NaN/Infinity, which might cause downstream issues in the AST. Consider error handling or special-casing zero divisors.
405-405: Ensure correct usage oftypeInfo.
ThetypeInfoproperty is introduced here but is leveraged in subsequent code. Verify that it’s consistently assigned before being accessed in other AST nodes.
441-457: Initialization logic seems solid.
These lines properly gather tokens intoidList. No immediate issues, but consider potential IR or memory overhead when lists grow large.
471-481: Function prototype structure is clear.
The identification of return type and parameters is straightforward. Ensure that call sites properly handle new or updated function signatures.
491-506: FunctionDeclarator logic reads well.
The approach of extractingheaderandparameterListis consistent with the parser design. No major concerns.
512-519: New scope creation inFunctionHeader.
Line 517 callssa.newScope(). Ensure every scope creation has a matching scope drop in all execution paths to prevent memory leaks or mis-scoped symbols.
564-587: ParameterDeclaration logic is consistent.
Binding parameters into the symbol table is correct. Consider carryingtypeQualifierforward if it affects variable usage (e.g.,const, etc.).
594-602: ParameterDeclarator usage is straightforward.
No immediate concerns. Implementation properly forms aSymbolTypefrom the children.
Line range hint
631-658: Validation of return statements inFunctionDefinition.
The refusal to compile functions missing a return statement for non-void types is logical and helps prevent runtime errors. The checks appear correct.
772-780: Builtin handling logic is clear.
Distinguishing builtin vs. custom function calls at parse time is beneficial. Ensure any expansions for builtins keep consistent naming.
1018-1032: Validate identifying tokens inStructSpecifier.
When there are exactly 6 children, the code setsthis.ident. If there's a scenario with a different child count or missing token, it might cause runtime errors.
1068-1101: Robust struct declaration logic.
Addingpropsbased on layout vs. normal struct declarations is well-structured. Watch for nested arrays or advanced usage that might arise in future expansions.
1109-1112: Validate array indexing when settingLayoutQualifier.index.
Line 1112 referenceschildren[4]. If the node shape changes, this assumption might break.
1118-1134: Collecting multipleStructDeclarators.
Implementation is consistent with the AST’s approach to list building. No immediate issues.
1141-1151: No major concerns inStructDeclarator.
The approach of storingidentandarraySpecifieris consistent with surrounding code.
Line range hint
1180-1202: Symbol resolution inVariableIdentifieris appropriate.
Error reporting for undeclared identifiers is good. Double-check that fallback ifsymbolInfois missing or built-in is comprehensive.
github-actions
bot
added
the
documentation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/shader-lab/src/parser/AST.ts (2)
209-222: Consider checking child node types for safer assignment.The logic in
FullySpecifiedTypeassignstypeSpecifierandqualifierListbased on the child node arrangement. If the grammar always guarantees that child 0 or child 1 will be of the correct type, this is fine. Otherwise, consider adding checks or error handling to avoid potential type mismatches. Overall, the direct property usage here seems clear and more performant than relying on getters.
317-321: Add a test to cover operator lexeme assignment.Line 321 updates
this.lexemebased on the operator token. According to static analysis, this line is not covered by tests. A test ensuring operators (e.g., +, -, *) are recognized and thatlexemeis set correctly would improve code coverage and confidence.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 321-321: packages/shader-lab/src/parser/AST.ts#L321
Added line #L321 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/shader-lab/src/lexer/Utils.ts(1 hunks)packages/shader-lab/src/parser/AST.ts(17 hunks)packages/shader-lab/src/preprocessor/PpScanner.ts(1 hunks)tests/vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- tests/vitest.config.ts
- packages/shader-lab/src/preprocessor/PpScanner.ts
- packages/shader-lab/src/lexer/Utils.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/parser/AST.ts
[warning] 321-321: packages/shader-lab/src/parser/AST.ts#L321
Added line #L321 was not covered by tests
[warning] 432-432: packages/shader-lab/src/parser/AST.ts#L432
Added line #L432 was not covered by tests
[warning] 444-445: packages/shader-lab/src/parser/AST.ts#L444-L445
Added lines #L444 - L445 were not covered by tests
[warning] 448-457: packages/shader-lab/src/parser/AST.ts#L448-L457
Added lines #L448 - L457 were not covered by tests
[warning] 740-740: packages/shader-lab/src/parser/AST.ts#L740
Added line #L740 was not covered by tests
[warning] 750-751: packages/shader-lab/src/parser/AST.ts#L750-L751
Added lines #L750 - L751 were not covered by tests
[warning] 1032-1032: packages/shader-lab/src/parser/AST.ts#L1032
Added line #L1032 was not covered by tests
[warning] 1129-1134: packages/shader-lab/src/parser/AST.ts#L1129-L1134
Added lines #L1129 - L1134 were not covered by tests
🔇 Additional comments (8)
packages/shader-lab/src/parser/AST.ts (8)
260-266: Verify token availability.Storing
qualifierandlexemeinBasicTypeQualifieris straightforward. However, ensure no scenarios exist wherethis.children[0]might be undefined or invalid. If guaranteed by the grammar, this is safe.
286-301: Direct property access looks good.Using
this.type,this.lexeme, andthis.isCustominTypeSpecifierclarifies the code, reducing reliance on getters. This should also improve performance slightly. No immediate issues found.
307-310: Validate array size.The
ArraySpecifierassignssizedirectly fromintegerConstantExpr.value. Consider validating that it’s a positive integer or handle unexpected scenarios (e.g., negative or non-numeric). Otherwise, future array usage could cause errors.
405-423: Ensure test coverage of declarator cases.In
InitDeclaratorList, multiple paths handle single and multiple declarations. Consider adding tests for these branches—especially around line 432 if there's a code path for array declarations or 2D arrays—so that the new logic is confirmed correct.
444-457: Test multiple-identifier scenario.
IdentifierListsupports multiple identifiers, yet lines 444-445 and 448-457 are uncovered by tests. A quick unit test can verify that repeated identifiers or multiple children are handled correctly, ensuring robust code coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 444-445: packages/shader-lab/src/parser/AST.ts#L444-L445
Added lines #L444 - L445 were not covered by tests
[warning] 448-457: packages/shader-lab/src/parser/AST.ts#L448-L457
Added lines #L448 - L457 were not covered by tests
740-758: Confirm fallback toTypeAnyis correct.In
FunctionCallParameterList, lines 740, 750-751 pushTypeAnywhenexpr.typeis undefined. Adding test coverage for these cases is recommended to ensure the fallback logic doesn't mask potential type issues. This also helps confirm that parameter signature logic remains consistent when missing or unknown types occur.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 740-740: packages/shader-lab/src/parser/AST.ts#L740
Added line #L740 was not covered by tests
[warning] 750-751: packages/shader-lab/src/parser/AST.ts#L750-L751
Added lines #L750 - L751 were not covered by tests
1032-1032: Add test coverage for unnamed struct scenario.At line 1032,
StructSpecifierhandles the case without an identifier. Please ensure a test covers struct declarations that lack an explicit name, so we confirm thatthis.propListis assigned correctly for both named and unnamed structs.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1032-1032: packages/shader-lab/src/parser/AST.ts#L1032
Added line #L1032 was not covered by tests
1129-1134: Thoroughly test multi-declarator struct definitions.
StructDeclaratorListmerges multiple declarators (lines 1129-1134), but static analysis indicates no test coverage for these lines. Tests covering multiple struct members in a single declaration would reduce regression risk.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1129-1134: packages/shader-lab/src/parser/AST.ts#L1129-L1134
Added lines #L1129 - L1134 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/case/project-loader.ts (2)
15-15: Optionally handle creation errors.
While this.thenchain suffices for normal execution, consider adding error handling with.catch()to manage potential failures gracefully.
21-22: Add error fallback for resource loading.
Including a.catch()block after.then()would improve reliability, ensuring that any network or parse failures are properly handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e/case/material-shaderLab.ts(1 hunks)e2e/case/project-loader.ts(1 hunks)e2e/case/shaderLab-mrt.ts(1 hunks)e2e/case/shaderLab-renderState.ts(1 hunks)examples/buffer-mesh-particle-shader-effect.ts(3 hunks)examples/shader-lab-multi-pass.ts(1 hunks)examples/shader-lab-simple.ts(3 hunks)examples/shader-lab.ts(3 hunks)examples/shader-water.ts(5 hunks)
✅ Files skipped from review due to trivial changes (7)
- e2e/case/material-shaderLab.ts
- examples/shader-lab-multi-pass.ts
- examples/shader-lab.ts
- examples/shader-water.ts
- e2e/case/shaderLab-renderState.ts
- examples/buffer-mesh-particle-shader-effect.ts
- examples/shader-lab-simple.ts
🔇 Additional comments (4)
e2e/case/project-loader.ts (3)
6-6: Consistent module import update.
Renaming@galacean/engine-shader-labto@galacean/engine-shaderlabaligns with your standardized naming convention across the codebase. Great job on ensuring consistency.
8-8: String delimiter alignment.
Using double quotes here is consistent with your overall string usage style changes. No issues found.
25-27: Camera retrieval looks good.
The logic for finding the"Camera"entity and invokinginitScreenshotis straightforward. No issues found.e2e/case/shaderLab-mrt.ts (1)
7-7: Maintaining uniform module references.
Switching to@galacean/engine-shaderlabaligns with the updated import path across the repo. Code remains functional with no further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/shader-lab/src/parser/AST.ts (6)
273-279: Initialize properties in the init method.Consider initializing the properties in the
initmethod to ensure proper cleanup when reusing instances from the object pool.abstract class BasicTypeQualifier extends TreeNode { qualifier: EKeyword; lexeme: string; + override init(): void { + this.qualifier = undefined; + this.lexeme = undefined; + } override semanticAnalyze(sa: SematicAnalyzer): void {
299-317: Consider type narrowing for better type safety.The type narrowing in the semantic analysis could be improved to ensure type safety.
override semanticAnalyze(sa: SematicAnalyzer): void { const children = this.children; const firstChild = children[0] as TypeSpecifierNonArray; this.type = firstChild.type; this.lexeme = firstChild.lexeme; - this.arraySize = (children?.[1] as ArraySpecifier)?.size; + const arraySpec = children[1]; + this.arraySize = arraySpec instanceof ArraySpecifier ? arraySpec.size : undefined; this.isCustom = typeof this.type === "string"; }
1084-1117: Add error handling for invalid struct declarations.Consider adding error handling for:
- Invalid type specifiers
- Duplicate property names
- Unsupported property types
override semanticAnalyze(sa: SematicAnalyzer): void { const children = this.children; + if (!children?.length) { + sa.reportError(this.location, "Invalid struct declaration"); + return; + } if (children.length === 3) {
1134-1150: Optimize list handling for better performance.The current implementation makes multiple array copies. Consider using array spread or concat for better performance.
override semanticAnalyze(sa: SematicAnalyzer): void { const children = this.children; if (children.length === 1) { this.declaratorList.push(children[0] as StructDeclarator); } else { const list = children[0] as StructDeclaratorList; - const declarator = children[1] as StructDeclarator; - for (let i = 0, length = list.declaratorList.length; i < length; i++) { - this.declaratorList.push(list.declaratorList[i]); - } - this.declaratorList.push(declarator); + this.declaratorList = [...list.declaratorList, children[1] as StructDeclarator]; } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1145-1150: packages/shader-lab/src/parser/AST.ts#L1145-L1150
Added lines #L1145 - L1150 were not covered by tests
Line range hint
1228-1250: Improve error handling for variable identifiers.Consider enhancing error reporting with more specific error messages and type information.
override semanticAnalyze(sa: SematicAnalyzer): void { const token = this.children[0] as Token; this.lexeme = token.lexeme; this.symbolInfo = sa.lookupSymbolBy(token.lexeme, ESymbolType.VAR) as VarSymbol; // #if _VERBOSE if (!this.symbolInfo) { - sa.reportError(this.location, `undeclared identifier: ${token.lexeme}`); + sa.reportError( + this.location, + `Undeclared identifier '${token.lexeme}'. Expected a variable declaration in the current scope.` + ); } // #endif this.typeInfo = this.symbolInfo?.dataType?.type; }
Line range hint
1-1267: Overall improvements needed for robustness and performance.Several areas need attention:
- Test Coverage: Multiple code paths lack coverage, particularly in:
- Array specifier handling
- Parameter list processing
- Struct declaration variants
- Performance: List operations could be optimized in multiple places
- Error Handling: Could be more informative and consistent
Consider:
- Adding a test coverage CI check
- Implementing a consistent error handling strategy
- Creating helper methods for common list operations
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1048-1048: packages/shader-lab/src/parser/AST.ts#L1048
Added line #L1048 was not covered by tests
[warning] 1145-1150: packages/shader-lab/src/parser/AST.ts#L1145-L1150
Added lines #L1145 - L1150 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/shader-lab/src/lexer/Utils.ts(1 hunks)packages/shader-lab/src/parser/AST.ts(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shader-lab/src/lexer/Utils.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/shader-lab/src/parser/AST.ts
[warning] 337-337: packages/shader-lab/src/parser/AST.ts#L337
Added line #L337 was not covered by tests
[warning] 460-461: packages/shader-lab/src/parser/AST.ts#L460-L461
Added lines #L460 - L461 were not covered by tests
[warning] 464-473: packages/shader-lab/src/parser/AST.ts#L464-L473
Added lines #L464 - L473 were not covered by tests
[warning] 756-756: packages/shader-lab/src/parser/AST.ts#L756
Added line #L756 was not covered by tests
[warning] 766-767: packages/shader-lab/src/parser/AST.ts#L766-L767
Added lines #L766 - L767 were not covered by tests
[warning] 1048-1048: packages/shader-lab/src/parser/AST.ts#L1048
Added line #L1048 was not covered by tests
[warning] 1145-1150: packages/shader-lab/src/parser/AST.ts#L1145-L1150
Added lines #L1145 - L1150 were not covered by tests
🔇 Additional comments (4)
packages/shader-lab/src/parser/AST.ts (4)
222-235: LGTM! Type-safe property declarations and semantic analysis.The changes improve type safety and maintainability by explicitly declaring properties and implementing proper semantic analysis.
323-326: Add test coverage for array size computation.The array size computation logic lacks test coverage. Consider adding test cases for:
- Valid integer constant expressions
- Edge cases (undefined size)
Would you like me to help create test cases for the ArraySpecifier class?
✅ Verification successful
Add test coverage for array size computation.
The verification confirms that there are no existing tests for the ArraySpecifier class and its array size computation logic. The review comment's recommendation for adding test coverage remains valid.
- No tests exist for the ArraySpecifier class in the test suite
- The semantic analysis functionality for array size computation is currently untested
- The test directory structure is properly set up at
tests/src/shader-lab/and can accommodate new test cases🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing tests of ArraySpecifier rg -l "ArraySpecifier" packages/shader-lab/test/Length of output: 114
Script:
#!/bin/bash # Find all test directories and files fd -t d "test" . fd -t f "test.*\.(ts|js)" .Length of output: 3488
Script:
#!/bin/bash # Check content of shader-lab test files and test cases rg "ArraySpecifier" tests/src/shader-lab/ cat tests/src/shader-lab/ShaderLab.test.ts ls -la tests/src/shader-lab/test-case/Length of output: 10107
1034-1048: Add test coverage for struct declaration variants.The else branch handling anonymous struct declarations (line 1048) lacks test coverage.
Would you like me to help create test cases for anonymous struct declarations?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1048-1048: packages/shader-lab/src/parser/AST.ts#L1048
Added line #L1048 was not covered by tests
743-774:⚠️ Potential issueAdd null checks and improve test coverage.
The parameter handling logic has several uncovered code paths and potential edge cases:
- No null checks for
expr.type- Uncovered code paths in the list handling logic
Add null checks:
override semanticAnalyze(sa: SematicAnalyzer): void { const { children, paramSig, paramNodes } = this; if (children.length === 1) { const expr = children[0] as AssignmentExpression; - if (expr.type == undefined) { + if (!expr?.type) { paramSig.push(TypeAny); } else { paramSig.push(expr.type); }Would you like me to help create test cases for the parameter list handling logic?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 756-756: packages/shader-lab/src/parser/AST.ts#L756
Added line #L756 was not covered by tests
[warning] 766-767: packages/shader-lab/src/parser/AST.ts#L766-L767
Added lines #L766 - L767 were not covered by tests
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
isPpCharactorstoisPpCharactersacross multiple files