ui: Migrating to Flechette for arrow record parsing#6176
Conversation
|
✅ Meticulous spotted visual differences in 9 of 349 screens tested, but all differences have already been approved: view differences detected. Meticulous evaluated ~5 hours of user flows against your PR. Last updated for commit c6f6bac. This comment will update as new commits are pushed. |
| const table: Table<any> = useMemo(() => { | ||
| const result = tableFromIPC(arrow.record); | ||
| const table: Table = useMemo(() => { | ||
| // Copy to aligned buffer only if byteOffset is not 8-byte aligned (required for BigUint64Array) |
There was a problem hiding this comment.
FYI I can't remember the exact configuration, but this is something we can enforce (at least in rust) on the backend.
Also, I first mistook this for a buffer only carrying the uint64 array, which is always naturally 8-byte aligned since every entry is 8-bytes, but this is because the record as a whole may have arrays that are not necessarily 8 byte aligned. This probably deserves some extra detail in the comment.
There was a problem hiding this comment.
Nevermind on the second part now that I read the second PR.
| return []; | ||
| } | ||
|
|
||
| // Use Set to collect unique filenames (Flechette decodes dictionaries upfront) |
There was a problem hiding this comment.
that's very unfortunate, it shouldn't matter in this case, since the backend should already return unique items, but maybe we should open an issue about this?
There was a problem hiding this comment.
Raise an issue to not decode the dictionaries upfront?
There was a problem hiding this comment.
As in, that we can directly access the underlying dictionary like we did before.
There was a problem hiding this comment.
I just found the underlying dictionary is already exposed, so changed this to use that one.
| const maxDepth = useMemo(() => { | ||
| if (callersFlamegraphData?.arrow != null) { | ||
| const table = tableFromIPC(callersFlamegraphData.arrow.record); | ||
| // Copy to aligned buffer only if byteOffset is not 8-byte aligned (required for BigUint64Array) |
There was a problem hiding this comment.
since this happens a couple times throughout the codebase now, maybe we can extract this into a util function?
3 participants
Changes in this PR should bring in significant(>2x) improvements in reading the arrow record data, but brings in a slight degraded(~0.7x) table parsing speed.
This degradation is caused by these array copies that are needed to align the bytes to support the bigints in the data.
Making a fix to improve that parsing performance in #6177 with speeds up the parsing by 1.3x.