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

Ability to delete data, alternatively setting a max window size and automatically trim oldest data #32

Closed
terjew opened this issue Jan 26, 2022 · 12 comments

Comments

@terjew
Copy link

terjew commented Jan 26, 2022

As the title says, I need some way of limiting the size of the dataset as the application keeps running. If I speed up the generation of data points, the application gets sluggish over time, presumably as more and more time is spent growing and copying the data buffer to make room for new data.

Is it possible to add support for trimming the beginning of the buffer, either on-demand or by setting a maximum buffer size? Perhaps the library could have an option where it uses a circular (ring) buffer for the data so no copying is needed?

@huww98
Copy link
Owner

huww98 commented Jan 26, 2022

Yes, I definitely want to support this. But I'm not sure about the API design. My current thought is to override the prototype of the data array, and hook into Array.prototype.splice(), Array.prototype.shift() and some other methods. Then I track the changes and sync them to GPU.

So to trim the oldest data, the users would write:

const data = [...];
const chart = new TimeChart(el, {
    series: [{ data }],
});

// trim 10 oldest datapoint
data.splice(0, 10);
chart.update();

this would be consistent with the current data.push(...) design, and is generic to also cover the prepending data use case (#8). The downside may include that I cannot support all array operations, which might cause confusion. And the rendering would be strange if the data is accidentally changed untracked.

How do you like this design?

@terjew
Copy link
Author

terjew commented Jan 26, 2022

This sounds like exactly what I need. Splicing the first x elements and then pushing the same amount of new entries is what the update loop would typically do anyway, so I think those two operations are the most relevant.

@terjew
Copy link
Author

terjew commented Jan 26, 2022

Instead of hooking the operations on the user-created array, I guess the time chart library could have its own data buffer class exposing only the supported operations.

@terjew
Copy link
Author

terjew commented Jan 30, 2022

@huww98 let me know if you have a test version of this that you would like me to try. I'm eager to see if this can make the library a viable option for my project, as far as I can see this is the only remaining showstopper.

@huww98
Copy link
Owner

huww98 commented Feb 1, 2022

@terjew Take a look at https://github.com/huww98/TimeChart/tree/wip-dynamic-data

The demo should work. But I have not carefully checked this feature would work in all cases.

@terjew
Copy link
Author

terjew commented Feb 17, 2022

@huww98

I finally had some time to try this properly for my project, and the good news is that it does indeed work. The bad news is that after pushing around 130k points (131073 seems to be the magic number) to the array, the rendering stops. The following error is observed in the javascript console:

image

This problem is also observed in the regular demo, when using the wip-dynamic-data branch. Just leave it running until it has produced 132k points and notice that the lines stop updating towards the right hand side.

@terjew
Copy link
Author

terjew commented Feb 17, 2022

Could this somehow be related to using a 17 bit counter somewhere? 2^17 is indeed 131072, one short of the magic number causing problems for me.

@huww98
Copy link
Owner

huww98 commented Feb 17, 2022

I allocate that much GPU memory at first. Then allocate a second chunk of memory if the first one is not large enough. There might be some bugs when dealing with more than one chunk of data.

@terjew
Copy link
Author

terjew commented Feb 17, 2022

You should be able to reproduce the bug simply by running the regular demo on the wip-branch and leave it running for around 2 minutes.

@huww98
Copy link
Owner

huww98 commented Feb 26, 2022

@terjew v1.0.0-beta6 is out. Please give it a try.

Also, could you help review the new doc about this? https://github.com/huww98/TimeChart#dynamic-data

@terjew
Copy link
Author

terjew commented Mar 13, 2022

@huww98 I finally had some time to try the latest beta, and I can confirm that it fixes the issue with rendering after 2^17 points added, awesome!

The documentation also looks good as far as I can see. My only issue at the moment is that the performance of the splice operation is quite bad. If I do a splice every frame as I add new points, the performance of my application with 3 dynamically updated charts falls from a stable 70 FPS (on a 70 Hz screen) to less than 30 FPS. If I instead do the splice only every once in a while, when the amount of points exceeds buffersize + tolerance I get a nice stable 70 FPS for the most part, but still with noticeable pauses whenever the tolerance is reached.

@huww98
Copy link
Owner

huww98 commented Mar 16, 2022

That needs more investigation. If it is the array splice method from the browser that causes the pause, we may need to implement our own ring buffer.

Anyway, we are able to delete data now.

@huww98 huww98 closed this as completed Mar 16, 2022
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

No branches or pull requests

2 participants