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

[rsc-compile] use already captured target sources snapshot instead of re-capturing #6700

Merged
merged 5 commits into from Nov 12, 2018

Conversation

Projects
None yet
4 participants
@ity
Member

ity commented Oct 29, 2018

Problem

See #6583 for details

@ity ity self-assigned this Oct 29, 2018

@ity ity requested a review from baroquebobcat Oct 29, 2018

@ity ity force-pushed the ity:ity/rsc_compile branch from 84d1e13 to 1d97717 Nov 5, 2018

@ity

This comment has been minimized.

Member

ity commented Nov 5, 2018

@baroquebobcat this is ready for review, whenever you have a moment :)

@baroquebobcat

Looks good to me!

@illicitonion

Looks good :) Thanks!

@@ -723,9 +721,12 @@ def _runtool(self, main, tool_name, args, distribution, tgt=None, input_files=tu
cmd.extend([main])
cmd.extend(args)
input_digest = self.context._scheduler.capture_snapshots((root,))[0].directory_digest

This comment has been minimized.

@illicitonion

illicitonion Nov 9, 2018

Contributor

It could be handy to avoid some no-op work here in some situations; in particular:

  • If pathglobs is empty, we shouldn't bother capturing a snapshot for it
  • If input_snapshot is None, or pathglobs was empty, we shouldn't bother merging directories
    (I haven't tested, but my assumption is that calling synchronously into the engine to ask it to capture or merge no-op things is non-trivial, albeit hopefully still cheap)

This comment has been minimized.

@stuhood

stuhood Nov 9, 2018

Member

I haven't tested, but my assumption is that calling synchronously into the engine to ask it to capture or merge no-op things is non-trivial, albeit hopefully still cheap

If I had to guess, I'd say it was on the order of tens of milliseconds... possibly not worth the special case.

baroquebobcat and others added some commits Nov 9, 2018

Update src/python/pants/backend/jvm/tasks/jvm_compile/rsc/README.md
Co-Authored-By: ity <ity@users.noreply.github.com>
@@ -699,7 +698,8 @@ def create_compile_context(self, target, target_workdir):
)
]
def _runtool(self, main, tool_name, args, distribution, tgt=None, input_files=tuple(), output_dir=None):
def _runtool(
self, main, tool_name, args, distribution, tgt=None, input_files=tuple(), input_snapshot=None, output_dir=None):
if self.execution_strategy == self.HERMETIC:
# TODO: accept input_digests as well as files.

This comment has been minimized.

@illicitonion

illicitonion Nov 12, 2018

Contributor

This is now done

# dont capture snapshot, if pathglobs is empty
input_digest = self.context._scheduler.capture_snapshots((root,))[0].directory_digest
if input_snapshot and input_digest:

This comment has been minimized.

@illicitonion

illicitonion Nov 12, 2018

Contributor

This will error if not pathglobs - need to explicitly set it to None or something `if not pathglobs

@ity ity merged commit d90005f into pantsbuild:master Nov 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment