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

Fix JVM fingerprinting by adding tags #8175

Merged
merged 3 commits into from Aug 16, 2019

Conversation

@wisechengyi
Copy link
Contributor

commented Aug 15, 2019

Problem

Tags could decide how a target is compiled (rsc, zinc), but we are not factoring them into the cache key, causing RscCompile task to hit the cache that should not be hit.

Solution

Explicitly add tags to target's fingerprint.

@wisechengyi wisechengyi changed the title [WIP] Fix JVM fingerprinting by adding tags and workflow info Fix JVM fingerprinting by adding tags and workflow info Aug 15, 2019

@wisechengyi

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

This is probably good for review as is if we want to get this fix asap, and tests can be a follow up.

@@ -133,6 +133,9 @@ def mirrored_target_option_actions(self):
def implementation_version(cls):
return super().implementation_version() + [('RscCompile', 173)]

def additional_confs_for_fignerprinting(self):

This comment has been minimized.

Copy link
@stuhood

stuhood Aug 15, 2019

Member

Hmm... this is supposed to be accounted for by

@memoized_property
def _build_invalidator(self):
return BuildInvalidator.Factory.create(build_task=self.fingerprint)
... is there a missing Subsystem dependency here?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 15, 2019

Contributor

It doesn't appear that workflow was added to the scala_library()'s payload, which is what I thought was used for fingerprinting:

payload = payload or Payload()
payload.add_fields({
'java_sources': PrimitiveField(self.assert_list(java_sources, key_arg='java_sources')),
})
.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 15, 2019

Contributor

Or in JvmTarget:

payload = payload or Payload()
excludes = ExcludesField(self.assert_list(excludes, expected_type=Exclude, key_arg='excludes'))
payload.add_fields({
'sources': self.create_sources_field(sources, address.spec_path, key_arg='sources'),
'provides': provides,
'excludes': excludes,
'platform': PrimitiveField(platform),
'strict_deps': PrimitiveField(strict_deps),
'exports': PrimitivesSetField(exports or []),
'compiler_option_sets': PrimitivesSetField(compiler_option_sets),
'zinc_file_manager': PrimitiveField(zinc_file_manager),
'javac_plugins': PrimitivesSetField(javac_plugins or []),
'javac_plugin_args': PrimitiveField(javac_plugin_args),
'scalac_plugins': PrimitivesSetField(scalac_plugins or []),
'scalac_plugin_args': PrimitiveField(scalac_plugin_args),
})

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Aug 15, 2019

Author Contributor

@stuhood that makes sense, although in practice changing --workflow resulted the same fingerprint, so likely something is missing.

This comment has been minimized.

Copy link
@stuhood

stuhood Aug 15, 2019

Member

@wisechengyi : Perhaps we're not fingerprinting the "enum" type in a useful way...?

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Aug 16, 2019

Author Contributor

Tried converting --workflow to a str type option, and target fingerprint isn't changed either, so likely it's just not taken into consideration.

This comment has been minimized.

Copy link
@wisechengyi

wisechengyi Aug 16, 2019

Author Contributor

Possibly false alarm, as I only printing the target key instead of the full key.

def key_for_target(self, target, transitive=False, fingerprint_strategy=None):
hasher = self._base_hasher.copy()
key_suffix = hasher.hexdigest()[:12]
if transitive:
target_key = target.transitive_invalidation_hash(fingerprint_strategy)
else:
target_key = target.invalidation_hash(fingerprint_strategy)
if target_key is not None:
full_key = '{target_key}_{key_suffix}'.format(target_key=target_key, key_suffix=key_suffix)
return CacheKey(target.id, full_key)
else:
return None

@@ -422,7 +425,7 @@ def execute(self):
classpath_product = self.create_runtime_classpath()

fingerprint_strategy = DependencyContext.global_instance().create_fingerprint_strategy(
classpath_product)
classpath_product, self.additional_confs_for_fignerprinting())

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 16, 2019

Contributor
Suggested change
classpath_product, self.additional_confs_for_fignerprinting())
classpath_product, self.additional_confs_for_fingerprinting())

@wisechengyi wisechengyi changed the title Fix JVM fingerprinting by adding tags and workflow info Fix JVM fingerprinting by adding tags Aug 16, 2019

@wisechengyi wisechengyi merged commit ea20d48 into pantsbuild:master Aug 16, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@wisechengyi wisechengyi deleted the wisechengyi:fix_rsc_finger branch Aug 16, 2019

patliu85 added a commit to twitter/pants that referenced this pull request Aug 20, 2019
Fix JVM fingerprinting by adding tags (pantsbuild#8175)
# Problem

Tags could decide how a target is compiled (rsc, zinc), but we are not factoring them into the cache key, causing RscCompile task to hit the cache that should not be hit.

# Solution

Explicitly add tags to target's fingerprint.

@stuhood stuhood added this to the 1.19.x milestone Aug 21, 2019

stuhood added a commit that referenced this pull request Aug 22, 2019
Fix JVM fingerprinting by adding tags (#8175)
# Problem

Tags could decide how a target is compiled (rsc, zinc), but we are not factoring them into the cache key, causing RscCompile task to hit the cache that should not be hit.

# Solution

Explicitly add tags to target's fingerprint.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.