JosephSilber / bouncer

Laravel Eloquent roles and abilities.
MIT License
3.43k stars 330 forks source link

Improve performance of CachedClipboard::findMatchingAbility #629

Closed equinoxmatt closed 7 months ago

equinoxmatt commented 1 year ago

As others have reported, the cachedClipboard is pretty inefficient and have some significant performance issues. In an attempt to find a quick win, I discovered that the findMatchingAbility method was using Arr::pluck(). Whilst this is a very powerful helper method, it is overkill in this context and is not efficient for this use case.

I have replaced Arr::pluck() with the more efficient mapWithKeys method, which, according to profiling results, has improved the performance by around 40%.

I did try and setup some benchmarks + run the tests, but the setup was pretty convoluted, so this may need testing before merge. However, all existing tests in our application, which cover the outcomes of Bouncer operations at a higher level, have passed successfully.

lrljoe commented 1 year ago

What was your approach for profiling out of interest.

Just about to take a look at this PR in a test system with some large quantities of data.

equinoxmatt commented 1 year ago

@lrljoe Mainly testing a variety of endpoints on our API which has a bunch of permissions checks. I used both Tideways & Blackfire for profiling the various requests and differences when the change was made. I did try and write some actual benchmarks, but the setup was convoluted and not worth the effort at the time. Curious to see what improvement you see, so please report back if you can.

lrljoe commented 1 year ago

Yeah it'll probably be Monday before I have any results, but should have something to show at least.

equinoxmatt commented 1 year ago

@lrljoe did you manage to see if this had an impact for you?

lrljoe commented 1 year ago

Ah jeez, apologies, this completely slipped my mind, give me a week and I'll come back with a definitive answer!

lrljoe commented 1 year ago

Ok. that week went by, will definitely review next week, bit hectic on some packages I help maintain with Livewire 3 being released as stable.

I do know that mapWithKeys will make an improvement in performance tho!