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

Introduce explicit cache writing job in RscCompile task #8190

Merged
merged 7 commits into from Aug 22, 2019

Conversation

@wiwa
Copy link
Contributor

commented Aug 20, 2019

Problem

Rsc outlining and zinc compiles of a target will race to write to the cache.
This will usually result in no zinc classes in the artifact due because zinc
will hit the cache during the cache double-check, causing runtime classpath to
be missing classes.

Solution

Create an explicit cache-write job dependent on the Rsc and zinc jobs
completing, removing the cache write from those jobs.

Result

Artifacts for rsc-and-zinc targets will deterministically contain both Rsc and
zinc classfiles as expected.

@stuhood stuhood requested review from cosmicexplorer and stuhood Aug 20, 2019

if not hit_cache:
counter_val = str(counter()).rjust(counter.format_length(), ' ')
counter_str = '[{}/{}] '.format(counter_val, counter.size)
self.context.log.info(

This comment has been minimized.

Copy link
@stuhood

stuhood Aug 20, 2019

Member

IMO, let's skip the output here (or move it to log.debug).

@@ -289,6 +289,9 @@ def _zinc_key_for_target(self, target, workflow):
'rsc-and-zinc': lambda: 'zinc[rsc-and-zinc]({})'.format(target.address.spec),
})()

def _cachewrite_key_for_target(self, target):
return 'cachewrite({})'.format(target.address.spec)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 21, 2019

Contributor
Suggested change
return 'cachewrite({})'.format(target.address.spec)
return 'write_to_cache({})'.format(target.address.spec)
@@ -386,6 +389,24 @@ def nonhermetic_digest_classpath():
# Update the products with the latest classes.
self.register_extra_products_from_contexts([ctx.target], compile_contexts)

def work_for_vts_cachewrite(vts, ctx):

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 21, 2019

Contributor
Suggested change
def work_for_vts_cachewrite(vts, ctx):
def work_for_vts_write_to_cache(vts, ctx):
def work_for_vts_cachewrite(vts, ctx):
# Double check the cache before beginning compilation
hit_cache = self.check_cache(vts, counter)
target = ctx.target

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 21, 2019

Contributor

This check is performed in multiple places in this method. Is it possible to factor out this check to reduce boilerplate (which matters since this is a very complex task)?

items_to_report_element([t.address.reference() for t in vts.targets], 'target'),
' (',
ctx.target.address.spec,
').')

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Aug 21, 2019

Contributor

Python 3 allows f-strings, which make this type of formatting much easier to follow. Additionally, I believe the elements such as items_to_report_element(ctx.sources, '{} source'.format(self.name())) are repeated elsewhere in this file as well. Is it possible to factor out that message templating into its own function?

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

@stuhood

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

(ftr: there are two legit failures in CI)

@stuhood
Copy link
Member

left a comment

Thanks a lot for diving in to write the test.

I have only nits, so don't bother fixing them unless you need another CI run anyway.

@stuhood

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

Talked with Danny about this, and he's fine with merging.

@stuhood stuhood merged commit 26e8056 into pantsbuild:master Aug 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
stuhood added a commit that referenced this pull request Aug 22, 2019
Introduce explicit cache writing job in RscCompile task (#8190)
### Problem

Rsc outlining and zinc compiles of a target will race to write to the cache.
This will usually result in no zinc classes in the artifact due because zinc
will hit the cache during the cache double-check, causing runtime classpath to
be missing classes.

### Solution

Create an explicit cache-write job dependent on the Rsc and zinc jobs
completing, removing the cache write from those jobs.

### Result

Artifacts for `rsc-and-zinc` targets will deterministically contain both Rsc and
zinc classfiles as expected.
stuhood added a commit that referenced this pull request Aug 23, 2019
stuhood added a commit that referenced this pull request Aug 29, 2019
Split out a double-check-cache job for jvm/rsc compile. (#8221)
### Problem

#8190 moved cache writing out of the completion of the zinc and rsc jobs and into a dependent job. But at the same time, we also had multiple attempts to "double check" the cache happening concurrently due to both the zinc and rsc jobs checking, and that race could lead to partial entries being extracted.

### Solution

Since we can't actually cancel or coordinate the concurrent work, we can't safely double check the cache once either job has started. So instead, this change extracts the cache double-check into its own job that both the zinc and rsc tasks will depend on.
stuhood added a commit that referenced this pull request Aug 29, 2019
Split out a double-check-cache job for jvm/rsc compile. (#8221)
### Problem

#8190 moved cache writing out of the completion of the zinc and rsc jobs and into a dependent job. But at the same time, we also had multiple attempts to "double check" the cache happening concurrently due to both the zinc and rsc jobs checking, and that race could lead to partial entries being extracted.

### Solution

Since we can't actually cancel or coordinate the concurrent work, we can't safely double check the cache once either job has started. So instead, this change extracts the cache double-check into its own job that both the zinc and rsc tasks will depend on.
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.