Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented a static cache making sure it's reset on object destruction #136

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

Toflar
Copy link
Contributor

@Toflar Toflar commented Dec 10, 2024

Alternative implementation to #130.

It's not really beautiful but static caches never are :/
@daun would love to get some feedback on the stats you get and also regarding the implementation of course.

@daun
Copy link
Contributor

daun commented Dec 10, 2024

Looks great! It's not that pretty, but if it's faster, who cares 🤠 Will run a few benchmarks in a minute.

One question: what's the reason the call to StaticCache::enterContext($this) must happen inside each method call to the Loupe instance? As opposed to doing it once in the constructor.

@Toflar
Copy link
Contributor Author

Toflar commented Dec 10, 2024

One question: what's the reason the call to StaticCache::enterContext($this) must happen inside each method call to the Loupe instance? As opposed to doing it once in the constructor.

Think of an application that works with two or more $loupe instances at the same time in the same process:

$loupe1->addDocument(...);
$loupe2->search(...);
$loupe3->addDocuments(...);
...

@daun
Copy link
Contributor

daun commented Dec 10, 2024

I see, that would allow switching back and forth without clearing the cache in between. Nice.

@daun
Copy link
Contributor

daun commented Dec 10, 2024

I'm seeing about a 1-2% speed bump for queries across the board. The very obvious question is of course if that's worth all the acrobatics and maintenance burden going forward.

What are you getting?

@Toflar
Copy link
Contributor Author

Toflar commented Dec 10, 2024

Similar 😊 I would argue that the work is done now anyway and I'm pretty sure there might be other use cases for a cache where we'd again needed such a static cache management. So I'd merge that for now but we can of course still remove it again if we have weird side-effects or encounter any other issues.

@daun
Copy link
Contributor

daun commented Dec 10, 2024

Alright, I'll close the other one.

@Toflar Toflar marked this pull request as ready for review December 10, 2024 16:17
@Toflar Toflar merged commit 7388a90 into develop Dec 10, 2024
18 checks passed
@Toflar
Copy link
Contributor Author

Toflar commented Dec 10, 2024

Thanks for the valuable feedback @daun!

@Toflar Toflar deleted the feature/cache-reset branch December 17, 2024 16:46
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.

2 participants