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

pass TokenCollection instead of Tokenizer to Metrics #208

Merged
merged 2 commits into from
Apr 5, 2016
Merged

pass TokenCollection instead of Tokenizer to Metrics #208

merged 2 commits into from
Apr 5, 2016

Conversation

pavarnos
Copy link
Contributor

for #188. Tokenizer was caching all the tokens for all files in the whole project. It didn't need to: each file is processed in turn and only the processed results are kept for analysis. Refactored to pass a TokenCollection to Metrics instead of the Tokenizer. Removed the Cache (no longer needed)

@pavarnos
Copy link
Contributor Author

to calculate peak memory usage, i added

        $output->writeln(memory_get_peak_usage(true) . ' bytes used'); 

to the end of RunMetricsCommand.php
but did not commit that change because it did not seem useful for normal users...

@@ -1,42 +0,0 @@
<?php
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you removed this file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was no longer used anywhere in the code base (i checked carefully). can easily put it back if you think it needed? I thought it is not a BC break because phpmetrics is an independent "app", not like a library with a public interface that needs a major version bump for a change like that... maybe i was wrong?

@Halleck45
Copy link
Collaborator

Hum, I prefer 2 PR:

  • one for this change (TokenCollection instead of Tokenizer)
  • one for removing dead code

Thanks a lot !

@@ -101,18 +100,18 @@ public function execute(ResultCollection $collection, ResultCollection $aggregat

// tools
$classMap = new ClassMap();
$tokenizer = new Tokenizer(new CacheMemory());
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pavarnos Cache is used at least here 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes... in the current master branch. But once this pull request is merged the code does not need it. (I searched through the whole code base: "CacheMemory" is not mentioned anywhere). Tokenizer now has no constructor.

@Halleck45
Copy link
Collaborator

@pavarnos you made a really hard work 👍. On my computer, PhpMetrics is around 20% faster 😎

I'll merge as soon you will revert your changes about cache.

@Halleck45
Copy link
Collaborator

Please be patient @pavarnos , I hope I'll merge tomorrow :)

@Halleck45 Halleck45 merged commit f910038 into phpmetrics:master Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants