#1 Performance measurement#2
Conversation
Beside adding the code, I did the following: - Minor style changes - Add MemoryStats class - Add statistics recording in ZDD classes
As system memory usages fluctuates over time, tool implementation, hardware, etc., measuring these statistics are 'removed' by commenting the out. If one (who knows what he is doing) still wants to measure the system memory usage, the code is still available. By removing system memory statistics, the statistics output is completely deterministic for the same input and bdd operations.
Now the caller of the BDD package should determine how to format the stats printing. We now just return the stats.
|
You changed |
|
I scrolled through it a bit and left some comments. Please feel free to address these already. I'll review it in more detail later. |
|
You did a bunch of things, besides adding platform-independent performance statistics. You also changed the |
|
I've finished my review, I think. Let me know what you think or if you have any questions. |
|
Hi @magoorden. Do you plan to work on this further? Or did my feedback scare you away? I'd love to hear from you. |
You did not scare me at all. I'm just at holiday now. So when I'm back, I'll take a closer look at your feedback. |
|
OK, no problem. Enjoy your holidays! |
Good question, haven't thought about that. Let me investigate it. It might be the case that all changes from BDDFactory move to JFactory in the end. |
Are your comments only the onces about the BDDFactory vs JFactory and the changelog? If there is more, I am not able to find them... |
That would be the least work. Not sure however whether that is the most logical choice from the JavaBDD perspective. |
I'm looking at #2 and I see 5 comments, then an indication that there are 15 hidden conversations, and then 5 more comments, as part of my review. The first one is about the changelog file, but there are thus many more. I then see some additional standalone comments, including about |
|
I guess I don't see the hidden conversations 😆 |
You don't see something like this when you scroll up from these comments we're now exchanging:
|
No, I don't. My comment counter at the top of the pages only counts 13. |
Mine is now also only 14 comments (one more than yours I guess due to you adding another comment). The screenshot that I shared is part of the review, not of the comments. What happens if you go to the 'Files changed' tab. Do you see my 'review comments' then? |
I can imagine only updating |
I'm still not able to make your review comments visible. They are not being shown in between the code, nor anywhere at the top in a form of a list.
|
It turns out I had to 'submit' my review. I think you can see the comments now? |
Yes, I can see them 😃 |
|
@dhendriks I have been through all your comments. Most of them are resolved. I left some of them open for further discussion. Once all comments are resolved, I plan to update |
I agree. |
|
I'm wondering about the maximum memory usage. The fact that Java employs garbage collection makes it difficult to correctly measure the maximum memory usage, as the peak may have between just before a garbage collection, between two measurements that we do. See for instance https://cruftex.net/2017/03/28/The-6-Memory-Metrics-You-Should-Track-in-Your-Java-Benchmarks.html for various approaches and why it is so difficult to get it right. So my question is: what is the practical purpose of this? Where would you use this for? And couldn't a user just use a tool like VisualVM for this? Maybe we should remove the memory statistics and let users do that themselves, using external means, where they control the environment and circumstances? Then we only count the platform-independent ones in the library itself. |
|
I've completed my review. |
- Allow stat measurements to be disabled - More consistent naming of stat objects and methods. - Move stats measurements at several places to a separate method.
Good point. I have removed the memory measurement part. |
|
@dhendriks Thanks for the review. I have adapted the code based on it. There are two comments unresolved. For those I have the suggestion to move your raised points to new issues to be dealt with later. I wonder whether you would agree or not. |
|
I've completed another review round. I think we're getting close.
I've added comments to those. I think those are resolved now. |
|
Your comments have been resolved. I also updated the |
dhendriks
left a comment
dhendriks
left a comment
There was a problem hiding this comment.
I've completed another review. I've added some suggestions for the changelog.
2 participants
This merge requests adds code to collect performance measurements.
Closes #1.