Add limitMetrics option for demo page to limit the total number of metric entries per class #1629
Conversation
|
@tchakabam I followed instructions to rebase the fork against the master and it ended up making the pull request look like I had 177 commits pending. Apparently, PRs don't like being rebased. |
|
This pull request introduces 3 alerts when merging e849f92 into 9b429dc - view on lgtm.com new alerts:
Comment posted by lgtm.com |
|
Hey @jon-valliere sorry for the long reply :) looks all good to me now! 👍 |
|
@tchakabam I thought you wanted me to reformat how the arrays were being added/trimmed or something? |
|
as far as i remember, and comparing to the current patch here, you did the exact change i was asking for meanwhile, which was that you called exactly one function each time to trim the array, while before you were calling two functions from the same stack depth level. you now call |
|
I didn’t make any changes. So, not sure what was going on. As long as you
are happy about it now. 😄
…On Thu, Apr 5, 2018 at 6:57 PM Stephan Hesse ***@***.***> wrote:
as far as i remember, and comparing to the current patch here, you did the
exact change i was asking for meanwhile, which was that you called exactly
one function each time to trim the array, while before you were calling two
functions from the same stack depth level. you now call
trimEventHistory();, while before it was trimEventHistoryA();
trimEventHistoryB(); in each place you called, not sure for what reason.
this now seems to make more sense.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1629 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AUOqevihvgxrLmU_3CAwwnAT-aaViWGNks5tlqF3gaJpZM4S7uym>
.
|
|
hey @jon-valliere meanwhile remembered what i had initially meant, and of course you are right, that issue that i noticed wasn't solved in the way this was merged here (not an urgent problem though). i meant basically that i think some of that code is quite redundant as we trim every time we add an event to the events array. you should have some generic method to "add" events and the logic inside that method would also do the trimming. but never mind, i plan to refactor this metrics events stuff anyway into something of it's own, some kind of extension, and break it out of the demo :) |
|
@tchakabam Yeah, a singular add method could also do the trimming; instead of push/pop it could move the elements when the limit is hit to reduce the cost of reallocating the entire array. Without the limit the cost of the vm copying the array during push becomes huge as well as the memory it starts to require. |
|
:) i m not that concerned about performance regarding this logic (i don't think there s a need for optimization except for the memory footprint, which was adressed here), more about code maintainability and avoiding redundance. what i meant was: if we would want to add triggering a new "event" now, we would have to be quite verbose, like push this into the array, trim it now, etc. it should be just like a method (pushData(type, data)) that's it. the trimming could even be done in a deferred async periodic task, why would the side pushing the data have to care about it. if we even want to call it an "event". to me what we collect here is more like data-points, some sort of time-series. because an event to me is something that one can attach to and propagates (many listeners can attach to many targets, events get dispatched to call listeners handlers in some way when they occur). i see that they are metrical events in some sort, sampling points of certain data-structures. i know many people call these events as well when tracking data from apps of some sort. but as we already have actual events in the lib i think this is really confusing and kinda ugly (but not your fault, just ranting about current state of things in general :). also (like in a lot of other places in Hls.js codebase) these data structures are declared implictely through object litterals. the problem is that we don't really define an interface or a proper type ever for these and it's also totally undocumented what are the semantics really, is there a base-class or something and what are the possible data structure types for each measurement. please don't take it badly, all these are just all general painpoints related to the lib but not related to your work here specifically of course, but this PR made the problems really point out again to me :) |
|
I too have a need for defining the correct nomenclature for things; however, an event is just something that happens at a point in time. You could call them Records or Entires but they're still based on Events. The whole viewer could be its own project. Its such a great way for viewing diagnostic information. |
|
yes, totally agree, sorry about the whole sermon ;) i totally know and agree with metrical data collection often using the term event. of course those records are based on events here as well, but to me they seem more like records :) because they are data we create when we handle the event. they are not just the event data we get from Hls.js. i mean basically that we should clearly differentiate therefore from the events we already have in Hls.js and their respective event data, as in this specific case it gets pretty confusing :D also what i realized is we should probably break out the metrics stuff in the demo here into some sort of own module that could be better maintainable. |
|
How about this: once the Event is stored, it becomes a Record. lol :) |
|
haha. yeah maybe one way to look at it. as i said i am not against storing data and calling it events in principle. but here is my problem: https://github.com/video-dev/hls.js/blob/master/demo/main.js#L384 we create data structures called events based on an event triggered by Hls.js. so to me that's not storing an event, like if we would plainly store the |
Description of the Changes
Add limitMetrics option for demo page to limit the total number of metric entries per class; this prevents memory leaks when using the demo player for long periods of time. Default is set to -1; which means Unlimited.