Skip to content

Add per-target zinc compile stats#4790

Merged
stuhood merged 5 commits into
pantsbuild:masterfrom
twitter:stuhood/add-compile-stats
Aug 9, 2017
Merged

Add per-target zinc compile stats#4790
stuhood merged 5 commits into
pantsbuild:masterfrom
twitter:stuhood/add-compile-stats

Conversation

@stuhood

@stuhood stuhood commented Aug 8, 2017

Copy link
Copy Markdown
Member

Problem

Visibility into the size and compile time of individual JVM targets is currently not great. While log parsing is possible, it's not scalable for this usecase.

Solution

Now that we have an API for recording target information, begin recording zinc compilation info.

Result

The target_data entry in the posted JSON stats contains information like the following:

 'src/scala/org/pantsbuild/zinc/analysis:analysis': {
    u'compile.zinc': {
        u'compile': {
            u'classpath_len': 54,
             'sources_len': 3,
             'incremental': False,
             'time': 6.252178907394409
        }
    }
},
 'src/scala/org/pantsbuild/zinc/extractor:extractor': {
    u'compile.zinc': {
        u'compile': {
            u'classpath_len': 61,
             'sources_len': 3,
             'incremental': False,
             'time': 6.599011182785034
        }
    }
},

... in addition to any data recorded by junit.

@stuhood stuhood requested review from benjyw and kwlzn August 9, 2017 02:50

@kwlzn kwlzn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm!

tgt, = vts.targets
fatal_warnings = self._compute_language_property(tgt, lambda x: x.fatal_warnings)
zinc_file_manager = self._compute_language_property(tgt, lambda x: x.zinc_file_manager)
start_time = time.time()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe use the Timer class from pants.util.contextutil?

@benjyw benjyw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe use the Timer class from pants.util.contextutil?

@baroquebobcat baroquebobcat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks pretty good!
Just a couple comments, but I don't consider them blocking.

  • Could you note specifically what info you are adding to the reporting in the summary?
  • Also, I'd like to see tests for this if possible.

@stuhood stuhood merged commit e9030aa into pantsbuild:master Aug 9, 2017
@stuhood stuhood deleted the stuhood/add-compile-stats branch August 9, 2017 22:14
@mateor

mateor commented Sep 10, 2017

Copy link
Copy Markdown
Member

I cannot wait to use this. Thanks stu!

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.

5 participants