Fix compile time verification performance regression for sqlite #1946
Conversation
|
Do you mind looking at the failing CI checks? |
liningpan
force-pushed
the
fix-sqlite-explain-performance
branch
from
4a06161 to
fbc796b
Compare
Update to 0.6.1 isn't possible right now due to massive increase in compile time and memory usage. The core build uses 30GB after a few minutes! With 0.5.13 the build takes less than a minute and < 2GB. See launchbadge/sqlx#1921 Possible fix with launchbadge/sqlx#1946
|
@liningpan do you have time to address the CI failures I mentioned before? The logs are gone now but if you amend your commit to update the SHA and push it to re-run the workflows we'll get new logs. |
|
Yeah, sorry. It's easy to get sidetracked and forget about PRs in the queue. I don't even have the "Approve and Run" button anymore though, you'll need to update the branch for it to come back I think. A simple rebase should do it. |
liningpan
force-pushed
the
fix-sqlite-explain-performance
branch
from
fbc796b to
92289c0
Compare
|
Although the CI pipeline appears to be passing, I'm not 100% certain about the correctness of this patch and the additional memory consumed by the branch state hash table. That being said it should still use way less memory than what's previously stored in the query history structure. I would also like to hear the original author's (sqlite CTE #1816) opinion @tyrelr. |
|
Overall, I think this is a good approach and it worked when I tested it. It is a bit unfortunate to need a 3rd way of storing column-states. But there is no simple way to avoid that due to HashMaps not implementing Hash (with good reason). Based on reading the code, the only piece of information that seems to be discarded is which register is pointed at by a PseudoCursor. That should be almost impossible to cause an issue in practice. Literally everything in the state except that pseudocursor would need to be identical to cause a mistaken short-circuit. Then, for that mistaken short-circuit to matter, the two nearly identical states would need to result in significantly different output data types. Such a query plan would be valid, but would be unlike any other query plan I've seen sqlite generate. |
|
@liningpan @tyrelr is it possible for this PR to cause type/null inference changes? |
|
In practice, I expect that a different result would never actually happen. It requires a very atypical query plan for the issue above to occur, I expect sqlite would never generate such a plan. But theoretically, this could change type/null inference if sqlite DID happen to generate that weird query plan. |
|
For safety, I've earmarked this for 0.7.0, which I've started working on. |
* add instruction, register, and cursor state memorization * fix: fixed formating
* add instruction, register, and cursor state memorization * fix: fixed formating
…chbadge#1946) * add instruction, register, and cursor state memorization * fix: fixed formating
The sqlite CTE feature (#1816) introduced severe performance degradation, and this PR attempts to fix it.
This patch drastically reduces search space by adding memorization based on register type and cursor data type. Pushing new states on to the Stack is guarded by a
HashSet, where if no new information is gained about an instruction (i.e. different register/cursor data type), the instruction will not be searched again.Now the example query in #1921 only takes 5s to verify instead of several minutes. It might be possible to further optimize this by stop tracking unnecessary information, which could save a lot of memory allocations.