Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Offload transformations onto separate thread #304

Merged
merged 8 commits into from
Nov 29, 2024

Conversation

richiejp
Copy link
Contributor

@richiejp richiejp commented Nov 25, 2024

  • sheet: Offload user transformations to background thread

Instead of doing the entire transformation in the foreground, this does an
initial chunk of the transformation in the foreground then offloads the rest
onto a background thread.

When new data is written to the transformation file the background thread
signals that new data is available and the UI buffer is reloaded.

The effect is that the user can see the transformation happen. If the index
were also built incrementally as new data became available then they could
start browsing the partially transformed file. This would require some work on
the index which currently just gets started after the transformation is finished.

As with doing the index in a background thread, this introduces a number of
problems. The background threads rely on UI buffer to store the mutex and other
resources, but the background threads are detached from the main thread and the
UI buffer is deleted on the main thread. So the background threads can outlive
the UI buffer.
(fixed)

Probably the UI buffer should not own the mutex and threads should not be
created on an ad-hoc basis, but instead be part of a thread pool that executes
a work queue or some similar scheme. This is probably beyond the scope of this
PR, but worth noting.

TODO:

  • fix bug which prevents the handler from being triggered and causes the status to disappear
  • add a cancellation point into the transformation loop
  • prevent UI buffer being free'd before the transformation has finished or been cancelled
  • prevent actions (e.g. search, filter) from being carried out on the buffer while transformation is in progress
  • convert filter to use the transformation API
  • add destructors to free user supplied resources (or find way to use the current buffer destructor)
  • fix failing extension test
  • fix display error when opening a particular 2 column file results in a third column being added with data from the first and second column
  • tests

@richiejp richiejp force-pushed the transformer-offload branch 5 times, most recently from 51e6b26 to 7cbe704 Compare November 26, 2024 14:33
@richiejp richiejp mentioned this pull request Nov 26, 2024
3 tasks
@richiejp richiejp marked this pull request as ready for review November 26, 2024 15:11
@richiejp richiejp force-pushed the transformer-offload branch 6 times, most recently from d5eeb93 to 6db3424 Compare November 28, 2024 10:25
@richiejp richiejp force-pushed the transformer-offload branch 4 times, most recently from 431d94a to e5d0d23 Compare November 28, 2024 11:00
@richiejp
Copy link
Contributor Author

One thing that would be nice to add to this is that the index is built with the transformation. Another is that as the index is made available while it is only partially complete. Then the user could begin to browse more than just the first buffer's worth of the file. However it could require some more intrusive changes and this PR is already quite large. @liquidaty

@richiejp richiejp force-pushed the transformer-offload branch 3 times, most recently from fbbbce8 to 86b7da6 Compare November 28, 2024 15:33
This uses the transformation API to produce a filter file and makes
some changes to the API to allow almost the same behavior that the feature
had before.

The behavior has changed because now executing filter repeatedly
filters the previously filtered file instead of replacing the filter.
@richiejp richiejp force-pushed the transformer-offload branch from 86b7da6 to 3f38d92 Compare November 28, 2024 15:50
@liquidaty
Copy link
Owner

One thing that would be nice to add to this is that the index is built with the transformation. Another is that as the index is made available while it is only partially complete. Then the user could begin to browse more than just the first buffer's worth of the file. However it could require some more intrusive changes and this PR is already quite large

Agreed, in particular the first-- will merge this PR and then can make that a separate new one

@liquidaty liquidaty merged commit af96426 into liquidaty:main Nov 29, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants