Repository navigation
Conversation
✅ Deploy Preview for electrodb-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
hey @tywalch 👋! finally got to work on the JIT optimization I shared with you a while ago. to give you an idea of the improvement, I have an API route, which takes in total under a second, that processes a lot of items, and I measured the total time spent in there's still more on the table. right now the doc client unmarshalls the raw DynamoDB response, which is also quite slow because it's megamorphic, and then ElectroDB walks the result again to format it. we can fold the unmarshalling into the JIT formatter so it will be monomorphic. in my benchmarks that unmarshall pass costs as much as ~50% of ElectroDB's normal formatting (in the same route, 88ms unmarshalling on top of the 175ms format). I manually tested a fused unmarshall + JIT formatter and it's also about ~95% faster. so combined, that's ~263ms of unmarshall+format CPU per request on this route today. I will try to work on the unmarshall stuff at some point, but even this PR should already provide much relief and reduce the cpu work done in the formatter. one nice thing about this PR is that the whole test suite now runs with thanks in advance for taking the time to review this 🙏 |
2049ecb to
73fd0cc
Compare
anatolzak
changed the title
|
@tywalch friendly nudge 🙂 |
| Compilation falls back to the interpreted path (unchanged behavior) when: | ||
|
|
||
| - The schema defines any user getter on an attribute. Custom getters run arbitrary code, so those schemas are never compiled. | ||
| - Runtime code generation is unavailable (`new Function` is blocked, e.g. under a strict CSP). In `off` or default mode this is a silent fallback; in `on` mode it throws `CompilationFailed`. |
There was a problem hiding this comment.
I believe we should fail loudly in all cases we can't support, in this case throwing a CompilationFailed error
There was a problem hiding this comment.
@tywalch hey! thank you so much for coming back to this. i really genuinely appreciate it. as far as what you said, I can see where you're coming from, but I tend to disagree here. Whether compilation works also depends on the runtime. For example, Cloudflare restricts new Function evaluation in ways normal runtimes don't. I don't want users to have to change the compile flag depending on the environment they're deploying to.
Also, compilation support depends on the shape of the entity. For example, we don't support getters right now, but when we add support for them later, I don't want the user to have to think about it. If we throw an error, they'd have to keep track of which entities are supported and which aren't.
Honestly, as a user, I'd just want it compiled if possible, and if not, then not. I don't want to have to figure out when it isn't supported and avoid enabling the flag so the whole instantiation doesn't crash. It's just best effort
I'm pretty sure this is how the other libraries do it too: Zod, Effect Schema, and Drizzle all fall back silently.
what do you think?
| | `off` | Disable compilation everywhere, ignoring `compile: true`. | | ||
| | `verify` | Run both the compiled and interpreted paths on every read and throw if their output diverges. | | ||
|
|
||
| `verify` is a correctness check for CI, not for production — it does the work of both paths. |
There was a problem hiding this comment.
My gut says we shouldn't have this, but I am interested in hearing more in case I'm missing something. Users should expect us to own and ensure that there is no functional divergence in their queries when using this feature. Unless the user can create a divergence themselves (I don't think they can), we should remove this option.
If this is a flag for internal testing, it would be better to have its own mechanism/env to avoid coupling it to a feature we've committed to supporting. Ideally, correctness diffs are done in test code rather than in the library, but let me know more about the challenges you encountered.
There was a problem hiding this comment.
yes it's only for internal testing so i totally agree with your point. I will check if we can get it done this way. I'll look at it tomorrow
There was a problem hiding this comment.
@tywalch addressed in the latest commit. let me know what you think
… in the shared paths Map
… into a test harness
73fd0cc to
1dd3e08
Compare
2 participants
Closes #585
What
Adds an opt-in, JIT-compiled document formatter for the read path (
get/query/scan/parse). Instead of re-walking the schema for every item through the interpreted formatter, ElectroDB compiles a formatter specialized to a single entity's schema once, atEntityconstruction, and reuses it. Output is byte-for-byte identical; only the per-item formatting cost changes.Design & safety
new Entity(schema, { table, client, compile: true }), or globally via theELECTRODB_COMPILEenv var.ELECTRODB_COMPILE=on, which forces compilation and throwsCompilationFailedwhere runtime codegen is unavailable (e.g. strict CSP,new Functionblocked).Correctness
ELECTRODB_COMPILE=verifyruns every read through both the compiled and interpreted paths and throws on any divergence. The full suite now runs under verify mode by default (npm run test:unit:verify, wired intotest:run), so both paths are exercised on every CI run in a single pass.The generated function body is snapshotted in
test/snapshots/compile/*.txtso codegen changes surface as a reviewable diff:npm run test:snapshots— verify the generated source hasn't drifted from the committed snapshots.npm run test:snapshots:update— regenerate the snapshots (review the diff to confirm the change makes sense, then commit).