Cache purge stub#6465
Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds a ctx.cache stub property to ExecutionContext, delegating to a virtual Worker::Api::getCtxCacheProperty() hook that returns undefined by default.
Issues (highest severity first):
- [HIGH] New
ctx.cacheproperty is unconditionally exposed with no compat flag or autogate. Adding a new property to everyExecutionContextinstance is a behavioral change — it could shadow user-definedcacheproperties or break code that checks'cache' in ctx. Otherctxproperties (exports,version,abort) are all gated behind compat flags. This needs at minimum a compat flag or autogate for staged rollout. - [MEDIUM] No tests. There are no tests covering the new
getCache()path or thegetCtxCacheProperty()hook. - [LOW] Missing TypeScript type definition. The
JSG_TS_OVERRIDEblocks inJSG_RESOURCE_TYPEdon't includecache, so generated TypeScript types won't reflect this property. - [LOW] Comment inaccuracies. Minor comment issues flagged inline.
These comments were generated by an AI code review assistant and may not be perfect. Please use your judgment when evaluating them.
|
Review posted successfully on PR #6465. Here's a summary of the findings:
|
|
The generated output of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6465 +/- ##
==========================================
- Coverage 70.82% 70.81% -0.01%
==========================================
Files 435 435
Lines 122656 122668 +12
Branches 19267 19268 +1
==========================================
+ Hits 86871 86872 +1
- Misses 24371 24383 +12
+ Partials 11414 11413 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8413fb6 to
5535f5e
Compare
|
@jasnell can you review this PR please, you probably know better than I do the intricacies that are worth watching out for here. |
9048e42 to
651cbfa
Compare
651cbfa to
a743801
Compare
Creates a ctx.cache object with a purge function. This allows Cache-enabled workers to purge their own cache. We expose ctx.cache as JSG_RESOURCE_TYPE with the purge function defined on it, and the types for that function as separate classes. By default when an IoContext is not present this field is undefined. However it is designed to be overridden when embedding workerd for actual use with the provided types.
a743801 to
b4828ae
Compare
Merging this PR will improve performance by 13.83%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | Encode_ASCII_32[TextEncoder][0/0/32] |
3.1 ms | 2.7 ms | +13.83% |
Comparing atewari/purge-jsrpc (b4828ae) with main (1f30941)
Footnotes
-
129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Creates a ctx.cache object with a purge function.
This allows Cache-enabled workers to purge their own cache.
We expose ctx.cache as JSG_RESOURCE_TYPE with the purge function defined
on it, and the types for that function as separate classes. By default
when an IoContext is not present this field is undefined. However it is
designed to be overridden when embedding workerd for actual use with the
provided types.