Skip to content

Conversation

@dktapps
Copy link
Member

@dktapps dktapps commented Jul 30, 2021

Since SubChunk is final (or should be), there's clear bounds on its functionality.
Since all the types of its fields are also sealed, we can safely assume that the PHP method wrappers of PalettedBlockArray and LightArray won't be overridden. This means it's possible to avoid calling hot methods like PalettedBlockArray::get() via the Zend VM, and instead directly call their C++ counterparts without going through PHP. This eliminates 1 function call for hot functions like SubChunk::getFullBlock() and SubChunk::setFullBlock().

A quick performance test using this code:

<?php

declare(strict_types=1);

$extensionUsed = class_exists(\pocketmine\world\format\SubChunk::class, false);
echo "Extension used: " . ($extensionUsed ? "YES" : "NO") . "\n";
require 'vendor/autoload.php';

function test() : void{
	$palette = new \pocketmine\world\format\SubChunk(1, []);
	for($x = 0; $x < 16; ++$x){
		for($z = 0; $z < 16; ++$z){
			for($y = 0; $y < 16; ++$y){
				$palette->setFullBlock($x, $y, $z, ord("0"));
			}
		}
	}

	$loops = 10000;
	$val = 0;
	$start = hrtime(true);
	for($i = 0; $i < $loops; ++$i){
		for($x = 0; $x < 16; ++$x){
			for($z = 0; $z < 16; ++$z){
				for($y = 0; $y < 16; ++$y){
					$val += $palette->getFullBlock($x, $y, $z);
				}
			}
		}
	}
	echo "Time: " . number_format((hrtime(true) - $start) / ($loops * 4096)) . "ns/op\n";
}
test();

yields the following results on Windows, PHP 8.0.8 (release), JIT=off:

Extension used: YES
Time: 53ns/op

compared to:

Extension used: NO
Time: 130ns/op

This shows a performance improvement of approximately 60%, with throughput improved by 2.5x.

@dktapps
Copy link
Member Author

dktapps commented Jul 31, 2021

I think this is now functionally OK, but it needs to be thoroughly covered by tests before merge.

The following things should be tested:

  • All public methods
  • Cloning
  • Serialize/unserialize
  • Garbage collection of empty palettes

layer->container.collectGarbage(false, 0);

bool keep = false;
for (auto p : layer->container.getPalette()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stupid as per pmmp/PocketMine-MP#6574, although the impact here is less than in PHP

@dktapps
Copy link
Member Author

dktapps commented Dec 16, 2024

The serialize code should be simplified. I've always hated doing it this way. get_properties_for handler could probably do it.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements the pocketmine\world\format\SubChunk class natively in C++ to improve performance by eliminating PHP function call overhead for hot path methods like getFullBlock() and setFullBlock(). The implementation achieves approximately 60% performance improvement (2.5x throughput) by directly calling C++ methods on PalettedBlockArray and LightArray objects without going through the Zend VM.

Key changes:

  • Native C++ implementation of SubChunk class with full support for serialization, cloning, and memory management
  • Refactored helper functions in PhpPalettedBlockArray and PhpLightArray to support code reuse
  • Build system updates to include the new source files

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
stubs/pocketmine/world/format/SubChunk.stub.php PHP stub file defining the SubChunk class interface for stub generation
stubs/pocketmine/world/format/SubChunk_arginfo.h Generated argument info header for SubChunk methods
src/PhpSubChunk.h Header file declaring the SubChunk class entry and registration function
src/PhpSubChunk.cpp Main implementation of SubChunk class in C++ including all methods, serialization, and memory management
src/PhpPalettedBlockArrayObj.h Exposes paletted_block_array_entry and paletted_block_array_fill function for use by SubChunk
src/PhpPalettedBlockArray.h Removes duplicate paletted_block_array_entry declaration (moved to PhpPalettedBlockArrayObj.h)
src/PhpPalettedBlockArray.cpp Refactors __construct to use new paletted_block_array_fill helper function
src/PhpLightArray.h Exposes light_array_obj struct, light_array_entry, and light_array_fill function for use by SubChunk
src/PhpLightArray.cpp Refactors fill method to use new light_array_fill helper function
config.w32 Adds PhpSubChunk.cpp to Windows build configuration
config.m4 Adds PhpSubChunk.cpp to Unix build configuration
chunkutils2.cpp Registers the new SubChunk class during module initialization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

RETURN_LONG(y);
}
}
RETURN_LONG(-1);
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value is set to -1, but according to the function signature this should return NULL when no highest block is found (return type is ?int). This case occurs when blockLayers is empty and should use RETURN_NULL() instead of RETURN_LONG(-1).

Copilot uses AI. Check for mistakes.
sub_chunk_obj* const new_object = fetch_intern(sub_chunk_new(object->ce));

new_object->emptyBlockId = old_object->emptyBlockId;
new(&new_object->blockLayers) std::vector<paletted_block_array_obj*>();
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The blockLayers vector is constructed twice without destroying the first instance. The vector is already initialized in sub_chunk_new() at line 39, and then it's constructed again here at line 51 using placement new without calling the destructor first. This creates a memory leak. Remove this redundant construction as the vector is already initialized.

Suggested change
new(&new_object->blockLayers) std::vector<paletted_block_array_obj*>();

Copilot uses AI. Check for mistakes.
zval zvNewLayer;
object_init_ex(&zvNewLayer, paletted_block_array_entry);
blockLayer = fetch_from_zend_object<paletted_block_array_obj>(Z_OBJ(zvNewLayer));
if (!paletted_block_array_fill(blockLayer, intern->emptyBlockId)) {
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory leak when paletted_block_array_fill fails. A new PalettedBlockArray object is created with object_init_ex at line 354 (refcount 1), the pointer is extracted and stored in blockLayer at line 355, but if paletted_block_array_fill returns false at line 356, the function returns early at line 357 without releasing the object or adding it to blockLayers. This orphans the object with refcount 1, causing a memory leak. The object should be released with OBJ_RELEASE before returning on failure.

Suggested change
if (!paletted_block_array_fill(blockLayer, intern->emptyBlockId)) {
if (!paletted_block_array_fill(blockLayer, intern->emptyBlockId)) {
OBJ_RELEASE(Z_OBJ(zvNewLayer));

Copilot uses AI. Check for mistakes.

auto intern = sub_chunk_this();
if (intern->blockLayers.empty()) {
RETURN_LONG(-1);
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value is set to -1, but the function signature indicates it returns a nullable integer (?int in PHP). The value should be returned as NULL instead of -1 to properly represent the absence of a highest block. The PHP stub shows the return type as ?int, which means null should be returned when no non-empty block is found.

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +37

public function getFullBlock(int $x, int $y, int $z) : int{}

Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getFullBlock() and setFullBlock() expose native accessors that take arbitrary int $x, int $y, and int $z coordinates without documenting or enforcing a valid range. In the underlying C++ implementation, these coordinates are passed straight into a fixed-size 16×16×16 block array that only relies on assert()s for bounds checks, so out-of-range or negative values are truncated and can index beyond the allocated storage, leading to out-of-bounds reads/writes and potential memory corruption. These methods should validate that x, y, and z are within the subchunk bounds (e.g. 0–15) and reject invalid inputs before delegating to the native container.

Suggested change
public function getFullBlock(int $x, int $y, int $z) : int{}
/**
* Returns the full block ID at the given subchunk-local coordinates.
*
* The coordinates must be within the subchunk bounds:
* - 0 <= $x <= 15
* - 0 <= $y <= 15
* - 0 <= $z <= 15
*
* Behaviour is undefined if coordinates outside this range are supplied.
*/
public function getFullBlock(int $x, int $y, int $z) : int{}
/**
* Sets the full block ID at the given subchunk-local coordinates.
*
* The coordinates must be within the subchunk bounds:
* - 0 <= $x <= 15
* - 0 <= $y <= 15
* - 0 <= $z <= 15
*
* Behaviour is undefined if coordinates outside this range are supplied.
*/

Copilot uses AI. Check for mistakes.
* @return PalettedBlockArray[]
*/
public function getBlockLayers() : array{}

Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getHighestBlockAt() accepts arbitrary int $x and int $z values and forwards them to the native subchunk implementation, which in turn indexes into a fixed-size 16×16×16 block array without runtime bounds checks. If a caller supplies out-of-range or negative coordinates, the underlying C++ code will truncate them to a byte and may read beyond the end of the array, which is undefined behaviour and can be abused for memory disclosure or corruption. This method should enforce that x and z are within the valid subchunk coordinate range before calling into the native container.

Suggested change
/**
* Returns the highest non-air block at the given local coordinates within this subchunk.
*
* @param int<0, 15> $x Local X coordinate within the subchunk (0-15 inclusive)
* @param int<0, 15> $z Local Z coordinate within the subchunk (0-15 inclusive)
*/

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant