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

[#22] Implemented simple CMS gc logs parser #23

Merged
merged 2 commits into from
May 18, 2016
Merged

[#22] Implemented simple CMS gc logs parser #23

merged 2 commits into from
May 18, 2016

Conversation

ducky-hong
Copy link
Collaborator

This is the simple CMS gc logs parser. It parses Full GC, CMS Initial Mark, CMS Final Remark, ParNew (Minor GC), ParNew -> Full GC. It does not support CMS-concurrent things yet.

Also changed gc_model.proto (user, sys, real type changed from float to double) by @JeOhLee 's request.

private static final Logger logger = LoggerFactory.getLogger(CmsLogParser.class);

private int currentThread;
private Map<Integer, String> threadToIncompleteLine = new LinkedHashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why you choose LinkedHashMap for this data-structure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No special reason. I would change it HashMap.

@xeoh xeoh added the task label May 12, 2016
@xeoh xeoh added this to the task milestone May 12, 2016
assertEquals(sys, event.getSysTime(), 0.0001);
assertEquals(real, event.getRealTime(), 0.0001);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ducky-hong
Copy link
Collaborator Author

I have changed parsing implementation to PEG. All tests passed.

@skyturtlerocket
Copy link
Collaborator

github가 아직 익숙치 않은데 이제는 commit 첫번째꺼는 리뷰하지 않아도 되는 건가요?

}

@Test
public void testParseFullGc() throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용자가 실행한 System.gc()는 FullGCSystem이라고 하죠. testParseFullGCSystem이라고 이름을 바꾸면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ducky-hong
Copy link
Collaborator Author

@habals
상단 탭의 Files changed 만 보시면 될 것 같습니다. 커밋을 두개로 나누었는데 regex -> PEG 로 바꾼 이력을 남기려고 한 것입니다. 크게 의미가 없으면 하나로 합쳐도 됩니다.
리뷰 감사합니다!

final String log = "<writer thread='11779'/>\n"
+ "126.743: [GC (Allocation Failure) 126.743: [ParNew: 30719K-&gt;30719K(30720K), 0.0000574 secs]126.743: [CMS"
+ ": 66466K-&gt;45817K(68288K), 0.2277434 secs] 97186K-&gt;45817K(99008K), [Metaspace: 60953K-&gt;60953K(1105920K)], 0.2280550 secs] [Times: user=0.23 sys=0.00, real=0.22 secs]\n"
+ "127.029: [Full GC 127.029: [CMS: 49837K-&gt;38462K(68288K), 0.1817762 secs] 65053K-&gt;38462K(99008K), [Metaspace: 60946K-&gt;60946K(1105920K)], 0.1819922 secs] [Times: user=0.18 sys=0.00, real=0.18 secs]";;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two semicolons?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. Thanks!

@xeoh
Copy link
Owner

xeoh commented May 17, 2016

LGTM.
According to today's class, in future, it would be better to change output of parse as immutable list from guava.

@dansuh17
Copy link
Collaborator

👍

parboiled is PEG (Parsing Expression Grammar) implementation.
PEG is more concise than regex, and regex could not handle recursive
structure well.

GcEventNode is added to access the parsed data easily. Its concrete class is
generated by Auto Value library which utilizes annotation processing. IDE setup
is required and the instruction can be found here:
    google/auto#106
@ducky-hong
Copy link
Collaborator Author

@JeOhLee
Thanks. I made the result list of parse() immutable.
If it looks good, can you merge this PR?

@xeoh xeoh merged commit 695ee46 into develop May 18, 2016
@xeoh xeoh deleted the iss22 branch May 18, 2016 10:14
@xeoh
Copy link
Owner

xeoh commented May 22, 2016

@ducky-hong This PR is vanished!! Is this Github Bug????? 😱

@xeoh xeoh restored the iss22 branch May 22, 2016 20:11
@xeoh xeoh deleted the iss22 branch May 22, 2016 20:15
@xeoh xeoh restored the iss22 branch May 22, 2016 21:17
@xeoh xeoh deleted the iss22 branch May 22, 2016 21:18
@ducky-hong ducky-hong restored the iss22 branch June 8, 2016 10:22
@ducky-hong ducky-hong deleted the iss22 branch June 8, 2016 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants