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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Hermetic zinc compile] Memoize scalac classpath snapshots #6491

Merged
merged 7 commits into from Sep 18, 2018

Conversation

Projects
None yet
4 participants
@dotordogh
Copy link
Contributor

dotordogh commented Sep 12, 2018

Completed with @baroquebobcat 馃槃

Problem

We are recalculating the snapshot for the scalac classpath. See #6435 for more info.

Solution

Memoize the snapshots.

Result

Fixed! 馃槂

@dotordogh dotordogh requested a review from illicitonion Sep 12, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great, thanks! Just a few minor clean-ups :)

if scheduler and scala_path:
list_of_classpath_entries = []
for path in scala_path:
snapshot = scheduler.capture_snapshots((PathGlobsAndRoot(

This comment has been minimized.

@illicitonion

illicitonion Sep 12, 2018

Contributor

Let's do one big batch snapshot, which can happen in parallel, rather than doing them one by one:

snapshots = scheduler.capture_snapshots(
  tuple(PathGlobsAndRoot(PathGlobs([path]), get_buildroot()) for path in scala_path)
)

and then zip snapshots and scala_path into ClasspathEntrys.

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 12, 2018

Contributor

That makes more sense.

I didn't know that it worked that way when we were looking at it yesterday. I followed the calls and now I see that the snapshots are futures::join_alled which preserves ordering. Cool!

Rust's docs are consistently pretty high quality and remind me of when I was working mostly in Ruby, but better. https://docs.rs/futures/0.2.0/futures/future/fn.join_all.html

list_of_classpath_entries.append(ClasspathEntry(path, snapshot))
return list_of_classpath_entries
else:
return [ClasspathEntry(p, None) for p in scala_path]

def compiler_classpath(self, products):

This comment has been minimized.

@illicitonion

illicitonion Sep 12, 2018

Contributor

There's now only one caller of this function - let's:

  1. Migrate the new caller to use the entries version
  2. Deprecate this version
  3. Maybe implemented this in terms of the old one (return [ce.path for ce in self.compiler_classpath_entries(products)]) rather than re-implementing it
compiler_classpath_entries = self._tool_classpath('scalac', products)
return [classpath_entry.path for classpath_entry in compiler_classpath_entries]

def compiler_classpath_entries(self, products, scheduler=None):

This comment has been minimized.

@illicitonion

illicitonion Sep 12, 2018

Contributor

My instinct would be to require a scheduler, rather than have it be optional, in the public interface; any strong reasons to make it optional?

PathGlobs(scala_path),
get_buildroot(),
),))[0]
if scalac_classpath_entries:

This comment has been minimized.

@illicitonion

illicitonion Sep 12, 2018

Contributor

Drop the if here? extend on an empty list is a no-op, which is fine :)

Dorothy Ordogh
@stuhood
Copy link
Member

stuhood left a comment

One correctness issue, but then this looks good.


@deprecated('1.12.0.dev0', 'Use compiler_classpath_entries instead.')

This comment has been minimized.

@stuhood

stuhood Sep 12, 2018

Member

This method was not marked :API: public, so the deprecation cycle should not be necessary.

snapshots = scheduler.capture_snapshots(tuple(PathGlobsAndRoot(PathGlobs([path]), get_buildroot()) for path in scala_path))
return [ClasspathEntry(path, snapshot) for path, snapshot in list(zip(scala_path, snapshots))]
else:
return [ClasspathEntry(p, None) for p in scala_path]

This comment has been minimized.

@stuhood

stuhood Sep 12, 2018

Member

In this branch of the if/else the scala_path will definitely be empty or None, so this case doesn't make sense.

return self._tool_classpath('scalastyle', products)
"""Returns classpath as paths for scalastyle."""
classpath_entries = self._tool_classpath('scalastyle', products)
return [classpath_entry.path for classpath_entry in classpath_entries]

This comment has been minimized.

@stuhood

stuhood Sep 12, 2018

Member

Since these aren't API public, I think you're good to inline them, if you want. Maintaining them this way though may be easier. Up to you!

This comment has been minimized.

@dotordogh

dotordogh Sep 14, 2018

Contributor

I prefer it this way.

Dorothy Ordogh added some commits Sep 14, 2018

Dorothy Ordogh
Dorothy Ordogh
Dorothy Ordogh
@stuhood
Copy link
Member

stuhood left a comment

One nit (I think). Feel free to land if it's intentional.

@@ -152,22 +151,14 @@ def _memoized_scalac_classpath(self, scala_path, scheduler):
if scala_path:

This comment has been minimized.

@stuhood

stuhood Sep 14, 2018

Member

This will now return None if there is no scala_path... intentional? Would be good to be explicit probably.

Dorothy Ordogh added some commits Sep 17, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Thanks! :)

@dotordogh dotordogh merged commit 906e4e5 into pantsbuild:master Sep 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dotordogh dotordogh deleted the dotordogh:donh/6435 branch Sep 18, 2018

baroquebobcat added a commit to twitter/pants that referenced this pull request Sep 19, 2018

baroquebobcat added a commit to twitter/pants that referenced this pull request Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment