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

StatisticsHelper doesn't generate "Garbage Generated" output #24

Open
dsmiley opened this issue Sep 17, 2018 · 3 comments · May be fixed by #25
Open

StatisticsHelper doesn't generate "Garbage Generated" output #24

dsmiley opened this issue Sep 17, 2018 · 3 comments · May be fixed by #25

Comments

@dsmiley
Copy link

dsmiley commented Sep 17, 2018

StatisticsHelper.java generates information about the garbage collector during a benchmark task.
Here's a snippet that winds up at the end of a ".stdout" log file:

Garbage Generated in Young Generation: 48913.992 MiB
Garbage Generated in Survivor Generation: 69.45353 MiB
Garbage Generated in Old Generation: 66.00384 MiB

Due to a bug, those numbers if done today will all be zero. StatisticsHelper.run calls Thread.currentThread().setDaemon(true); which looked wrong to me, and sure enough was causing an IllegalStateException on the thread (or something like that if I recall). A Runnable being invoked by an Executor ought not to mess with the state of it's thread. And I think it's unnecessary to try to make these threads daemons since stopStatistics() will cancel then shut it down.

Note quite related: I also thought it prudent to explicitly call run() from stopStatistics so we get the very latest information.

Another aside: it's suspicious to see all these volatile fields and furthermore "+=" operations on them. I don't think there are thread-safety concerns; they can all be regular fields. I don't think there needs to be synchronization in this class either. Calling executor.shutdown() triggers a happens-before relationship with the state modified by the task thread.

I could submit a PR if desired.

CC @shaie whom I suspect wrote StatisticsHelper originally

@mikemccand
Copy link
Owner

+1 for a PR! Thanks @dsmiley.

dsmiley added a commit to dsmiley/luceneutil that referenced this issue Sep 20, 2018
…read

And update memory status one final time in stopStatistics()

Closes mikemccand#24
@dsmiley
Copy link
Author

dsmiley commented Mar 4, 2020

Might you look at the PR please?

@mikemccand
Copy link
Owner

Thanks for the ping @dsmiley; I'll have a look.

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 a pull request may close this issue.

2 participants