Patch 1#1
Merged
Merged
Conversation
Unfortunately, these new formatting codes conflict with the Java strikethrough and underline, so we can't support these anymore. A TextFormat::javaToBedrock() is provided to strip these codes, or (if these formats become supported via different codes) to convert them to Bedrock variants. Co-authored-by: Dylan T. <dktapps@pmmp.io>
Automatic merge performed by: https://github.com/pmmp/RestrictedActions/actions/runs/12307996607
This PR replicates the mechanism by which PHP's own GC is triggered: using a dynamically adjusted threshold based on the number of roots and the number of destroyed cycles. This approach was chosen to minimize behavioural changes. This currently only applies to the main thread. Doing this for other threads is a bit more complicated (and in the case of RakLib, possibly not necessary anyway). By doing this, we can get more accurate performance profiling. Instead of GC happening in random pathways and throwing off GC numbers, we trigger it in a predictable place, where timings can record it. This change may also produce minor performance improvements in code touching lots of objects (such as `CraftingDataPacket` encoding`), which previously might've triggered multiple GC runs within a single tick. Now that GC runs wait for `MemoryManager`, it can touch as many objects as it wants during a tick without paying a performance penalty. While working on this change I came across a number of issues that should probably be addressed in the future: 1) Objects like Server, World and Player that can't possibly be GC'd repeatedly end up in the GC root buffer because the refcounts fluctuate frequently. Because of the dependency chains in these objects, they all drag each other into GC, causing an almost guaranteed parasitic performance cost to GC. This is discussed in php/php-src#17131, as the proper solution to this is probably generational GC, or perhaps some way to explicitly mark objects to be ignored by GC. 2) World's `blockCache` blows up the GC root threshold due to poor size management. This leads to infrequent, but extremely expensive GC runs due to the sheer number of objects being scanned. We could avoid a lot of this cost by managing caches like this more effectively. 3) StringToItemParser and many of the pocketmine\data classes which make heavy use of closures are afflicted by thousands of reference cycles. This doesn't present a major performance issue in most cases because the cycles are simple, but this could easily be fixed with some simple refactors.
No one in their right mind is going to change the defaults for these anyway. All this crap does is overwhelm users with stuff they don't understand. Most of this stuff has no business being modified by non-developers anyway.
This has been a long-standing issue since at least 2016, and probably longer. Heavy use of getBlock(At) could cause the cache to blow up and use all available memory. Recently, it's become clear that unmanaged cache size is also a problem for GC, because the large number of objects blows up the GC root buffer. At first, this causes more frequent GC runs; later, the frequency of GC runs drops, but the performance cost of them goes up substantially because of the sheer number of objects. We can avoid this by trimming the cache when we detect that it's exceeded limits. I've implemented this in such a way that failing to update blockCacheSize in new code won't have lasting impacts, since the cache count will be recalculated during scheduled cache cleaning anyway. Closes pmmp#152.
async workers still use automatic GC for now. We should probably switch to manual GC at some point, but it's not a priority right now.
…ilar to the main thread this avoids costly GC runs during hot code.
This has bitten me on the ass so many times now
GC is not required for RakLib as it doesn't generate any unmanaged cycles. Cycles in general do exist (e.g. Server <-> ServerSession), but these are explicitly cleaned up, so GC wouldn't have any useful work to do.
Automatic merge performed by: https://github.com/pmmp/RestrictedActions/actions/runs/12440895736
this can happen due to very noisy outputs during conversion, e.g. if there were many unknown blocks.
this reduces the risk of OOM during conversion of large worlds we probably ought to limit the size of region caches for regionized worlds, but that's a problem for another time.
…romise this reduces memory footprint slightly, but more importantly reduces GC workload. Since it also reduces the work done on cache hit, it might *slightly* improve performance, but any improvement is likely to be minimal.
Automatic merge performed by: https://github.com/pmmp/RestrictedActions/actions/runs/12487960388
Automatic merge performed by: https://github.com/pmmp/RestrictedActions/actions/runs/12498201064
Automatic merge performed by: https://github.com/pmmp/RestrictedActions/actions/runs/12510342531
Automatic merge performed by: https://github.com/pmmp/RestrictedActions/actions/runs/12615442245
NBT has better quality type info already
the conventional way is using array_keys and array_fill_keys. Behaviour is more predictable & also avoids benevolent union fuckery from PHPStan.
and workaround PHPStan stupidity
this is a BUT (int|string) under PHPStan, and we don't need the errors. We don't care about this code anyway.
I don't think we get much benefit from this, and the assumption that functions with a return value are pure is sketchy. In any case, it's better to avoid these repeated calls anyway.
Since this doesn't directly correspond to flight speed (it's multiplied by different values depending on whether sprinting or not, and possibly other states), "multiplier" was preferred instead of directly calling it flight speed. Default value is 0.05.
Good thing this isn't saved on disk :|
Automatic merge performed by: https://github.com/pmmp/RestrictedActions/actions/runs/12839491049
Automatic merge performed by: https://github.com/pmmp/RestrictedActions/actions/runs/12899221905
…ctable" This reverts commit 97c5902.
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 71 files out of 153 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the 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 (
|
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues & PRs
Changes
API changes
Behavioural changes
Backwards compatibility
Follow-up
Tests