perf: experiment with an alternate binary search implementation #9852
Conversation
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
| @@ -231,6 +232,12 @@ export default class<TEnv extends Env = Env> extends WorkerEntrypoint<TEnv> { | |||
| pathname: string, | |||
| _request?: Request | |||
| ): Promise<string | null> { | |||
| const BINARY_SEARCH_EXPERIMENT_SAMPLE_RATE = 0.5; | |||
There was a problem hiding this comment.
I'd consider starting off with a lower threshold. 5%? Just make sure no errors are thrown — then we can jump up to more accurately compare performance.
There was a problem hiding this comment.
I'd consider starting off with a lower threshold. 5%? Just make sure no errors are thrown — then we can jump up to more accurately compare performance.
That is something I actually wanted to discuss with you and I think makes complete sense.
Optimizations: - remove recursion to use a while loop instead - do not allocate unnecessary array (to reduce GC) - convert the manifest to a Uint8Array once upfront - added a faster vesrion of compare (compareSearchValueWithEntry). The compare implementatiopn was moved to the tests where it is used Other changes - BS: use indexes instead of offest (IMO easier to read/debug) - BS: return the content hash instead of the whole entry
|
@GregBrimble I have rebased to fix an issue with the CI (the main branch was failing when I branched off). I have reduced the experiment to 5% of the request for now and added a fallback to the current impl. Let me know if it looks good. Thanks! |
Experiment with an alternate binary search implementation for asset.
The new implementation is ~10% faster in test.
The PR has 2 commits for easier review:
The first commit has the changes to the algorithm.
The second commit add the experiment with both the current and the updated version.
/cc @GregBrimble