Revised changes

[gp-lnuff]

Fixes #179 and addresses #178

This approach keeps the original functionality of the SelectTree intact by handling the $resultMap building differently.
Instead of branching the logic if the initial pass doesn't yield root results, we can use a second array as a cache for the hierarchical relationship and integrate the $resultMap as the iteration comes across the parents, confirming they are in the $results.

Making the map this bit* more expensive to build has another added benefit - if the relationship query itself is filtered/scoped, the tree will now correctly treat records whose parents don't show up in the $results as root results, without having to define $modifyQueryUsing or changing $parentNullValue.

I wonder if there is a case for only using one query to fetch the results, since designating root results is delegated to the $resultMap.

About that I have a question about intended usage, is the $parentNullValue supposed to be a value that another record should never have? I can smell the hint of a couple use cases where by setting $parentNullValue to the current record's Id or a related record's could let us build trees to pick "deep children of the current record".

I'd also like @Baspa to give some input about whether this addresses his concerns thoroughly.

* actual performance difference not tested

[Baspa]

Thank you very much! I will try to take a look at it tomorrow @gp-lnuff

[Baspa]

This fork seems to work for me @gp-lnuff CC @CodeWithDennis

[CodeWithDennis]

Could storing the results like this cause any performance issues with big trees? @gp-lnuff

[gp-lnuff]

Compared to the original algorithm I think the array_merges bump it over O(n), but I could probably refactor the one on line 218 as a union operator, and instead of array_mergeing the array_values of the $orphanedResults on line 246 I think a foreach loop applying a union between $resultMap[$parent] and the "$orphanedResult" array would also improve performance. I'm not exactly sure it would make it O(n), as I'm a little inexperienced when it comes to Big-O analysis. I'll try to run some benchmarks

[gp-lnuff]

It turns out the first array_merge was a downright mistake that would lead to an Undefined array key error, since the key in the $resultMap is only set in that very same if block. I refactored the second one, but haven't gotten any benchmarking done since I'm having some trouble coming up with synthetic, large data sets.

[gp-lnuff]

I'm done with the benchmarking.

I used this CategoryFactory class:

<?php

namespace Database\Factories;

use App\Models\Category;
use Illuminate\Database\Eloquent\Factories\Factory;

/**
 * @extends \Illuminate\Database\Eloquent\Factories\Factory<\App\Models\Category>
 */
class CategoryFactory extends Factory {
    /**
     * Define the model's default state.
     *
     * @return array<string, mixed>
     */
    public function definition(): array {
        return [
            "description" => fake()->uuid(),
            "category_id" => Category::factory(),
        ];
    }
}

with this CategorySeeder database seeder:

<?php

namespace Database\Seeders;

use App\Models\Category;
use Illuminate\Database\Seeder;
use Illuminate\Support\Facades\DB;

class CategorySeeder extends Seeder {
    /**
     * Seed the application's database.
     */
    public function run(): void {
        $baseDatasetSize = 20;
        $baseMaxNesting = 8;
        $recursiveCreateRandomChildren = function (Category $category, $upperBound) use (&$recursiveCreateRandomChildren) {
            if ($upperBound == 0) {
                return;
            }
            $rand = rand(0, $upperBound);
            $categories = Category::factory()->count($rand)->recycle($category)->create();
            $categories->each(fn($item) => $recursiveCreateRandomChildren($item, $upperBound - 1));
        };
        Category::factory($baseDatasetSize)->withoutParents()->create()->each(fn($item) => $recursiveCreateRandomChildren($item, $baseMaxNesting));
    }
}

To generate around 24'000 records in a hierarchical structure with varying levels of nesting.

I initially used much bigger numbers, which led me to write upwards of 4'000'000 records and that led to Fatal errors upon get()ting the queries, so I tried with lazy collections instead, but I was still exceeding memory limits upon allocating the $resultMap (This happens with the current implementation as well, I verified).

To benchmark the performance of the algorithm implementation, I used the \Illuminate\Support\Benchmark class in select-tree.blade.php in the following way:

# More code...
 x-data="selectTree({
            name: @js($getName()),
            state: $wire.{{ $applyStateBindingModifiers("\$entangle('{$getStatePath()}')") }},
            options: @php
                [$value, $duration] = Benchmark::value(fn() => $getTree());
                debug("$duration ms");
                echo Js::from($value)->toHtml();
            @endphp,
            searchable: @js($isSearchable()),
# More code...

The results for the 24'000-records tree were as such:

• Current implementation: 1360-1390 ms
• Proposed implementation: 1660-1690 ms

After the initial loading, performing any action on the tree (opening the select, expanding or collapsing a branch) would be noticeably delayed (in the ~1s range), happening on both current and proposed implementation which suggests that it's a performance issue on the javascript side.

I should note that at this size for the dataset, using a LazyCollection made no difference. It seems the javascript performance hits a ceiling before maximum allocated memory size becomes an issue. I would still keep this change in the PR as in the case of stored results, a LazyCollection should save resources.

Here are some other numbers, achieved by tweaking $baseDatasetSize and $baseMaxNesting:

With 7572 results ( $baseDatasetSize = 8 and $baseMaxNesting = 8):

• Current implementation: 405-415 ms`
• Proposed implementation: 485-500 ms

With 5178 results ( $baseDatasetSize = 20 and $baseMaxNesting = 7):

• Current implementation: 270-280 ms
• Proposed implementation: 330-340 ms

With 1606 results ( $baseDatasetSize = 20 and $baseMaxNesting = 6):

• Current implementation: 87-91 ms
• Proposed implementation: 102-105 ms

With 631 results ( $baseDatasetSize = 15 and $baseMaxNesting = 5):

• Current implementation: 36-37 ms
• Proposed implementation: 41-42 ms

With 913 results ( $baseDatasetSize = 30 and $baseMaxNesting = 5):

• Current implementation: 50-52 ms
• Proposed implementation: 59-61 ms

With 3160 results ( $baseDatasetSize = 120 and $baseMaxNesting = 5):

• Current implementation: 168-171 ms
• Proposed implementation: 200-208 ms

In conclusion, it looks like the performance of this implementation is worse with larger data sets, but it's close enough that the javascript side will start having issues before the difference between the two becomes noticeable by users. With the added benefit of handling #178 I would say I am satisfied with the performance. Otherwise, I would welcome further optimizations if anyone has suggestions.

[CodeWithDennis]

Wow.. Great work! — really appreciate the time you put into this!

Performance looks fine to me. I’ve run into a similar issue before where large trees can slow things down a bit. The only real way to fix that would be to avoid loading child items until a dropdown is opened — but since that’s not supported by the underlying package, it’s not something we can implement here either. @gp-lnuff

Overall, this looks like a solid fix. Hopefully, @Baspa can also take a look and see if it resolves the issues he was facing.

[Baspa]

Do you want me to check this? @CodeWithDennis

[CodeWithDennis]

Do you want me to check this? @CodeWithDennis

Yes please!

[CodeWithDennis]

@Baspa

[Baspa]

Still seems to work for me :) @CodeWithDennis

[CodeWithDennis]

Thank you both @gp-lnuff @Baspa