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

Make EventDescription's deletable #31

Closed
wants to merge 1 commit into from

Conversation

dedmen
Copy link
Contributor

@dedmen dedmen commented Mar 27, 2018

In my usecase I am profiling the script running inside a scriptVM. So the same function can belong to many different script files. Thus I can't make the EventDescription static per function.
I solved that by just having one EventDescription per file but after a reset in game every script is recompiled and I want to clear all EventDescriptions instead of creating memory leaks everywhere.

Maybe you have a better idea on how to solve this? I first wanted to just add a Event::Delete on https://github.com/bombomby/brofiler/pull/31/files#diff-71975803275f8f3cc69ffb081a77aad7R274
but I found out that each Description has an index corresponding to their position in the board (https://github.com/bombomby/brofiler/pull/31/files#diff-60b37754b374b6cbb51603dfd1148165R78)
array which means I would have to change all indeces.

Also may I ask why your code looks so old? No ranged for loops?

@bombomby
Copy link
Owner

Hi dedmen, Brofiler's EventDescriptions designed in a way to provide minimum possible runtime overhead. Usually they are created on demand as static local variables the first time you hit a profiling macros. It's kind of a permanent "string" table with all the existing function names.

As you've noticed this approach is not really good for "dynamic" descriptions (user-generated) which you need in your case.

The problem with your current solution is that it deletes all the EventDescriptions (including static local descriptions generated for each PROFILE macro). So deleting all the descriptions and then starting a new profiling session will lead to a crash.

There is two possible solutions to your problem:
1) Shared EventDescriptions
You could add inside scriptVM a shared container - std::unordered_map<std::string, const EventDescription*>. Then after recompiling the script you could just generate-and-add-to-the-cache or reuse EventDescriptions with the same name if they already in the map. In this case you'll loose File and Line for each of the EventDescriptions as you won't be able to reuse them - don't know whether this is critical in your case or not. As a bonus with this approach - you could even profile script reload mechanism. You will always have all the descriptions alive.

2) Temporary EventDescriptions
You could add a flag "bool isTemporary;" to the EventDescription. Then you could pass this flag in the constructor for all the EventDescription that you create in the script. After this changing DeleteAllDescriptions to DeleteAllTemporaryDescriptions should do the work.

@dedmen
Copy link
Contributor Author

dedmen commented Mar 29, 2018

  1. Problem is having to change the id member variable of each EventDescription so that they are in order again without holes in them. That's why I opted for a quick deleteAll. But yeah. deleteTemporary would be easier. I just have to make sure that all the static's are at the start and the temps at the end. That way I don't need to reorder.

  2. Already thought about solution 1 yeah. I'm storing the EventDescriptions in a custom VM instruction currently and just let them leak when it goes out of scope. I think the shared approach could work.
    I'll continue thinking about this. I'm currently in the "throw everything together and make it work" phase.

I also have another patch that checks if a EventDescription has been used and if not don't transmit it to the UI and just transmit empty strings instead. Do you think that's worth for your use case? In your use case the names and filepaths are probably quite short. But in my case I also store the full script that the EventDescription belongs to inside it.

Also have some more fixes on the UI. I'll try to get the PR's out over the weekend.

@bombomby
Copy link
Owner

Yeah, I forgot to mention that Brofiler uses stored id's only for serialization when you stop the capture.
So the easiest way to "reorder" stuff inside DeleteAllTemporaryDescriptions function is just to create an array on the stack, fill it with non-temp descriptions (also delete the temp descriptions in the same loop) and then swap arrays. After this you could just iterate over the new array and enumerate EventDescriptions from 0 to N-1.

@bombomby bombomby closed this Mar 9, 2019
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.

None yet

2 participants