add type hashing #8
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Startup
participant CompilerGlobals
participant ExecutorGlobals
Startup->>CompilerGlobals: Allocate & initialize constraint_cache
CompilerGlobals->>ExecutorGlobals: Assign constraint_cache in init_executor
ExecutorGlobals->>System: Ready with initialized constraint_cache
sequenceDiagram
participant InheritanceHandler
participant ConstraintCache
participant ClassEntry
InheritanceHandler->>ConstraintCache: Compute hash via zend_hash_type(type)
alt Constraint exists in cache
ConstraintCache-->>InheritanceHandler: Return constraint
else Not exists
InheritanceHandler->>ConstraintCache: Allocate new HashTable and cache it
end
InheritanceHandler->>ClassEntry: Verify type constraint satisfaction
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 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 (
|
![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)
Zend/zend_execute.c (4)
1165-1167: Verify handling of the return value fromzend_type_satisfied_by_class(...).The call to
zend_type_satisfied_by_class(original_type, ce);returns a boolean, but its result is not used here. If this is intentional (e.g., for side effects like caching), consider adding a short comment to clarify why you’re not checking or storing that result, ensuring future maintainers understand the design intent.
1178-1179: Usetrueinstead of1to express boolean return.Since the function returns a
bool, returningtrueinstead of1would improve clarity and consistency with the rest of the code.Apply this small change:
- return 1; + return true;
1188-1189: Consider DRYing up repeated intersection-type checks.This block repeats a call to
zend_check_intersection_type_from_cache_slot(...)in the same function. If possible, refactor to avoid duplication, or add a comment describing why it is necessary to repeat the call with different parameters and conditions.
1197-1197: Avoid ignoring the boolean return value fromzend_type_satisfied_by_class(type, ce).Like the earlier usage, the return value from
zend_type_satisfied_by_class(...)is not captured. If this call is solely for side effects, consider a comment clarifying that the result is intentionally discarded. Otherwise, you may want to check or propagate the boolean outcome for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Zend/zend.c(4 hunks)Zend/zend_compile.c(2 hunks)Zend/zend_execute.c(5 hunks)Zend/zend_execute_API.c(1 hunks)Zend/zend_globals.h(2 hunks)Zend/zend_inheritance.c(1 hunks)Zend/zend_inheritance.h(1 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- Zend/zend_compile.c
🧰 Additional context used
🧬 Code Definitions (3)
Zend/zend_inheritance.h (3)
Zend/zend_execute.c (10)
bool(242-249)bool(809-828)bool(1031-1046)bool(1048-1056)bool(1090-1100)bool(1171-1240)bool(1242-1259)bool(1261-1266)bool(1268-1282)bool(1284-1294)Zend/zend_execute_API.c (2)
bool(725-728)bool(1160-1168)Zend/zend_inheritance.c (3)
bool(2044-2077)bool(2208-2215)bool(3879-3901)
Zend/zend_inheritance.c (1)
Zend/zend_inheritance.h (7)
zend_type_satisfied_by_class(32-32)ce(38-38)ce(42-42)ce(46-46)ce(47-47)zend_hash_type(30-30)zend_type_is_satisfied_by_class(31-31)
Zend/zend_execute.c (1)
Zend/zend_inheritance.h (6)
ce(38-38)ce(42-42)ce(46-46)ce(47-47)zend_type_satisfied_by_class(32-32)zend_type_is_satisfied_by_class(31-31)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: FREEBSD
- GitHub Check: BENCHMARKING
- GitHub Check: LINUX_X32_DEBUG_ZTS
🔇 Additional comments (14)
Zend/zend_globals.h (2)
97-97: Implementation of constraint_cache in compiler_globals looks good.This adds a new HashTable to store type constraints cache in the compiler globals structure, which will be used for optimizing type constraint checks during compilation.
195-195: Implementation of constraint_cache in executor_globals looks good.Adding the constraint cache to executor globals structure will make it accessible during runtime execution. This mirrors the same field in compiler_globals ensuring consistency between compile-time and runtime.
Zend/zend.c (4)
727-729: Proper initialization of constraint_cache.The constraint cache is properly initialized as a HashTable with size 64 and no specific destructor. The allocation and initialization follow the same pattern as other caches in this function.
800-806: Complete and correct cleanup of constraint_cache.This implementation correctly handles the destruction of the constraint cache:
- Iterates through each entry in the cache
- Destroys each HashTable entry
- Frees the allocated memory
- Destroys and frees the main cache itself
This ensures proper memory management and prevents memory leaks.
1042-1042: Memory allocation for constraint_cache during startup.This line allocates memory for the constraint_cache during the compiler globals initialization phase, following the pattern used for other hash tables in this function.
1138-1139: Proper cleanup during post-startup.These lines ensure the constraint_cache is properly freed and set to NULL during the post-startup phase, consistent with other cleanup operations in this function.
Zend/zend_execute_API.c (1)
151-151: Sharing constraint_cache between compiler and executor globals.This line ensures that the executor globals point to the same constraint_cache as compiler globals, consistent with how function_table and class_table are handled. This allows runtime execution to access the type constraints cached during compilation.
Zend/zend_inheritance.h (1)
30-32: New type hashing and validation functions.These functions add important capabilities for type handling:
zend_hash_type: Creates a hash from a zend_type structure (marked as FASTCALL for optimization)zend_type_is_satisfied_by_class: Checks whether a class satisfies a specific type constraintzend_type_satisfied_by_class: Registers a class as satisfying a type constraintThese functions will improve type checking performance by caching type validation results, particularly in class inheritance scenarios.
Zend/zend_inheritance.c (3)
2197-2206: Implementation of type constraint caching.This function records that a type is satisfied by a given class by storing the class name in a constraint cache. It uses a hash based on the type pointer to identify the constraint.
2208-2215: Check added for type constraint satisfaction.Good implementation that checks if a type has been satisfied by a class by looking up the constraint in the cache and verifying the class existence.
2217-2220: Simple type hashing implementation.The function uses the memory address of the type as its hash value. This is efficient but relies on memory address uniqueness for the lifetime of the type.
Zend/zend_execute.c (3)
1143-1143: Add documentation or checks for the neworiginal_typeparameter.A new parameter
zend_type *original_typehas been introduced to this function signature. It may be worth updating any associated comments or doc blocks to clarify its purpose and ensure that the value passed in is valid (e.g., not NULL). If there's a scenario in whichoriginal_typecould beNULL, consider adding defensive checks.
1147-1147: Initialization ofcetoNULLis fine but double-check usage flow.Declaring
zend_class_entry *ce = NULL;at this point is fine for clarity, but verify that it's always assigned before use. Ensure that error paths do not rely on an uninitializedce.
1184-1184: Confirm usage oftypevs.original_type.Here, the function forwards
typeas the first argument tozend_check_intersection_type_from_cache_slot(...).Verify that this is consistent with the new parameter's intended usage, rather thanoriginal_type, to avoid any subtle mismatch in logic or data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Zend/zend_execute.c (3)
1165-1167: Consider handling the return value fromzend_type_satisfied_by_class.
Currently, the result ofzend_type_satisfied_by_classis ignored. If it returnsfalse, we’re not logging or acting on it. Verify if the function call is for side effects only or if the boolean result needs to be utilized.
1184-1184: Directly returning from intersection type check.
Returning immediately helps performance by skipping further checks. Confirm that no additional coverage is needed for subsequent logic (e.g., fallback union branches).
1197-1197: Unused return value fromzend_type_satisfied_by_class.
The function’s boolean return is never checked. If the intention is purely side-effect-based, clarify with comments; otherwise, ensure correct handling of a potentialfalseoutcome.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Zend/zend_execute.c(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: WINDOWS_X64_ZTS
- GitHub Check: MACOS_ARM64_DEBUG_NTS
- GitHub Check: LINUX_X64_DEBUG_ZTS_ASAN
- GitHub Check: LINUX_X64_RELEASE_NTS
- GitHub Check: LINUX_X32_DEBUG_ZTS
- GitHub Check: BENCHMARKING
- GitHub Check: FREEBSD
🔇 Additional comments (3)
Zend/zend_execute.c (3)
1143-1143: Addedoriginal_typeparameter in function signature.
This new parameter appears essential for additional checks, but ensure non-null usage is always valid. Verify if extra validations or null checks are necessary to avoid unexpected crashes.
1177-1180: Early return on class satisfaction.
This snippet correctly short-circuits ifzend_type_is_satisfied_by_classis true, avoiding unnecessary checks. However, ensure no required side effects are skipped by this early return.
1188-1188: Short-circuit for union case with intersection type.
If this call succeeds, the code returns without proceeding. Ensure the function’s behavior is correct for other union parts and confirm no unintended skipping of needed validations.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor