rfc:hook_improvements

Differences

This shows you the differences between two versions of the page.

Link to this comparison view

Both sides previous revisionPrevious revision
Next revision		Previous revision
rfc:hook_improvements [2024/06/29 14:19] – Make performance gains more vague crellrfc:hook_improvements [2024/07/31 01:09] (current) – crell
Line 1: Line 1:
 ====== PHP RFC: Property hook improvements ====== ====== PHP RFC: Property hook improvements ======
   * Version: 0.9   * Version: 0.9
-  * Date: 2024-06-28 (use today's date here)+  * Date: 2024-06-28
   * Author:  Ilija Tovilo (tovilo.ilija@gmail.com), Larry Garfield (larry@garfieldtech.com)   * Author:  Ilija Tovilo (tovilo.ilija@gmail.com), Larry Garfield (larry@garfieldtech.com)
-  * Status: Under Discussion+  * Status: Implemented
   * First Published at: http://wiki.php.net/rfc/hook_improvements   * First Published at: http://wiki.php.net/rfc/hook_improvements
  
 ===== Introduction ===== ===== Introduction =====
  
-While finalizing the implementation of the [[rfc:property-hooks|property hooks RFC]] patch, we have found a few small areas for improvement.  These are nominally changes from the behavior defined in the original RFC, and thus we want to refer them through the RFC process.+While finalizing the implementation of the [[rfc:property-hooks|property hooks RFC]] patch, we have found a small area for improvement.  This is nominally changes from the behavior defined in the original RFC, and thus we want to refer them through the RFC process.
  
 ===== Proposal ===== ===== Proposal =====
  
-There are two improvements we propose. +There is one improvement we propose.
- +
-==== Remove the proactive guard against recursive backing value access ====+
  
 As defined in the original RFC, the following code would have been detected and throw a dedicated error. As defined in the original RFC, the following code would have been detected and throw a dedicated error.
Line 39: Line 37:
 While looking for optimizations, Ilija found he was able to refactor the hook logic to make it more than twice as fast: While there is still some overhead to a hook compared to a traditional get method, it's been reduced from "close to the cost of <php>__get</php>" to "almost negligible compared to a method."  Enough that it would almost certainly be swamped by the cost of the hook body itself.  This is a massive improvement. While looking for optimizations, Ilija found he was able to refactor the hook logic to make it more than twice as fast: While there is still some overhead to a hook compared to a traditional get method, it's been reduced from "close to the cost of <php>__get</php>" to "almost negligible compared to a method."  Enough that it would almost certainly be swamped by the cost of the hook body itself.  This is a massive improvement.
  
-A side effect of that optimization, however, is that we cannot proactively detect the bug above.  Instead, it would result in an infinite loop, which would eventually trigger a a stack overflow.+A side effect of that optimization, however, is that we cannot proactively detect the bug above.  Instead, it would result in an infinite loop, which would eventually trigger a stack limit exception from PHP.  (That logic already exists.)
  
 We consider the slightly worse error handling fully justified by the performance improvement, and by the fact that the code above is a bug no matter what (it's just a question of when the bug gets reported).  But as it is a change from the behavior stated in the previous RFC, we are putting it out for consideration. We consider the slightly worse error handling fully justified by the performance improvement, and by the fact that the code above is a bug no matter what (it's just a question of when the bug gets reported).  But as it is a change from the behavior stated in the previous RFC, we are putting it out for consideration.
  
-==== Partial support for readonly properties ==== 
- 
-Support for hooks on ''readonly'' properties was omitted from the original RFC, primarily to minimize complexity as there were questions around when it was safe to do.  On further consideration, we believe that hooks on backed properties are sufficiently safe to support readonly, but not on virtual properties. 
- 
-We therefore propose to allow both ''get'' and ''set'' hooks on ''readonly'' properties, if and only if it is a backed property. 
- 
-The main concern is that readonly implies a property is immutable and idempotent.  However, a ''get'' hook supports arbitrary code, so technically a developer could do something silly like: 
- 
-<code php> 
-class Dumb 
-{ 
-    public readonly int $value { get => random_int(1, 100); } 
-} 
-</code> 
- 
-On the other hand, there is no shortage of dumb things that people can do with PHP already.  The exact same silliness could be implemented using <php>__get</php>, for instance.  However, there are valid uses for a readonly get hook, especially for ORMs and proxies.  For example: 
- 
-<code php> 
-readonly class ProductFromDB 
-{ 
-    public string $id; 
-     
-    public function __construct( 
-        public string $name, 
-        public float $price, 
-        public Category $category, 
-    ) {} 
-} 
- 
-// Generated code. 
-readonly class LazyProduct 
-{ 
-    // Assigned via reflection or a closure. 
-    private DbConnection $dbApi; 
-     
-    // Assigned via reflection or a closure. 
-    private string $categoryId; 
- 
-    public Category $category { 
-        get { 
-            $this->category ??= $this-dbApi->loadCategory($this->categoryId); 
-        } 
-    } 
-} 
-</code> 
- 
-That is, we feel, an entirely reasonable use of hooks, and would allow for lazy-load behavior per-property on readonly classes. 
- 
-This is subtly different from the Lazy Proxy RFC, which operates on the whole object at once.  We believe both use cases are valuable and should be supported. 
- 
-A ''set'' hook, meanwhile, offers no issue for a backed readonly property.  As long as it is backed we are able to determine if it is still uninitialized, and so a second set call would correctly fail as it should. 
- 
-On balance, we believe the advantages and use cases for a lazy readonly property outweigh the potential for developers to do silly things.  For that reason, we propose to allow both get and set hooks on backed readonly properties. 
  
 ===== Backward Incompatible Changes ===== ===== Backward Incompatible Changes =====
Line 110: Line 55:
 ===== Proposed Voting Choices ===== ===== Proposed Voting Choices =====
  
-There will be two independent primary votes, one for each of the improvements suggested above.  Please vote for each separately.+ 
 +<doodle title="Remove the extra hook recursion guard?" auth="crell" voteType="single" closed="true"> 
 +   * Yes 
 +   * No 
 +</doodle>
  
 ===== Patches and Tests ===== ===== Patches and Tests =====
rfc/hook_improvements.1719670753.txt.gz · Last modified: 2024/06/29 14:19 by crell

Page Tools