Dan's latest changes to diagnose slow commits.#2492
Conversation
melvin-bot
bot
requested review from
srikarparsi
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8fedec260
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
libstuff/sqlite3.c
Outdated
| static sqlite3_uint64 hctRdtscTime(void){ | ||
| unsigned int lo, hi; | ||
| __asm__ __volatile__ ("rdtsc" : "=a" (lo), "=d" (hi)); | ||
| return (sqlite_uint64)hi << 32 | lo; |
There was a problem hiding this comment.
This new hctRdtscTime() uses unguarded GCC inline rdtsc, but HCT_ENABLE_COMMIT_TIMERS is defined unconditionally in this amalgamation. On non‑x86 targets (e.g., arm64) or non‑GCC compilers, this will fail to compile because rdtsc/inline asm is unsupported. Previously the sqlite3Hwtime() helper selected a safe fallback or architecture-specific implementation. Consider restoring the guarded helper or adding the same architecture checks here.
Useful? React with 👍 / 👎.
libstuff/sqlite3.c
Outdated
| static sqlite3_uint64 hctDbRdtscTime(void){ | ||
| unsigned int lo, hi; | ||
| __asm__ __volatile__ ("rdtsc" : "=a" (lo), "=d" (hi)); | ||
| return (sqlite_uint64)hi << 32 | lo; |
There was a problem hiding this comment.
The DB timer macro now depends on hctDbRdtscTime() which is hardwired to rdtsc via GCC inline asm. Because HCT_ENABLE_COMMIT_TIMERS is always enabled in this file, this introduces the same portability/build break on non‑x86 or non‑GCC toolchains. The prior sqlite3Hwtime() implementation included architecture fallbacks; this function should be similarly guarded or replaced.
Useful? React with 👍 / 👎.
|
Hey @tylerkaraszewski, do the above comments matter? I'm not too familiar with this area of the code |
|
@srikarparsi - I'm addressing with dan @ sqlite. I'll ping you agan when resolved, thanks! |
|
@srikarparsi - this is ready for re-review. Discussed with Dan and all we need is a compiler setting change instead. |
2 participants
Details
Just the latest version of SQLite from Dan
Fixed Issues
https://github.com/Expensify/Expensify/issues/558782
Tests
Internal Testing Reminder: when changing bedrock, please compile auth against your new changes