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.

Sign up for GitHub

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

Jump to bottom

Introduce explicit cache writing job in RscCompile task #8190

Merged

stuhood merged 7 commits into pantsbuild:master from wiwa:f/updatejob

Aug 22, 2019

Contributor

wiwa 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.


          Introduce explicit cache writing job in RscCompile task

stuhood requested review from cosmicexplorer and stuhood

August 20, 2019 23:42

stuhood reviewed

View reviewed changes

Member

stuhood left a comment

Thanks!

Would you mind adding a test in https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_cache_compile_integration.py ?

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py Outdated

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

Member

stuhood Aug 20, 2019

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

cosmicexplorer requested changes

View reviewed changes

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py Outdated

@@ @@ -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)

Contributor

cosmicexplorer Aug 20, 2019

Suggested change

      
                return 'cachewrite({})'.format(target.address.spec)
          
                return 'write_to_cache({})'.format(target.address.spec)

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py Outdated

@@ @@ -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):

Contributor

cosmicexplorer Aug 20, 2019

Suggested change

      
                def work_for_vts_cachewrite(vts, ctx):
          
                def work_for_vts_write_to_cache(vts, ctx):

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py Outdated

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

Contributor

cosmicexplorer Aug 20, 2019

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)?

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py Outdated

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

Contributor

cosmicexplorer Aug 21, 2019

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 added the needs-cherrypick label

stuhood added this to the 1.19.x milestone

Member

stuhood commented Aug 21, 2019

(ftr: there are two legit failures in CI)

Win Wang added 5 commits

August 21, 2019 13:12


          address easy feedback

c2b992d


          factor out cache double-check

b00b289


          update test_rsc_compile

b65924f


          wip cache test

b8d6455


          fixup! wip cache test

2c06d6f

stuhood approved these changes

View reviewed changes

Member

stuhood left a comment •

edited

Loading

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.

tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_cache_compile_integration.py Outdated Show resolved Hide resolved


          Fix bugs and add type hints to _compile_spec

5ea7803

Member

stuhood commented Aug 22, 2019

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

stuhood merged commit 26e8056 into pantsbuild:master

stuhood pushed a commit that referenced this pull request


          Introduce explicit cache writing job in RscCompile task (#8190)

fed6677

### 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 removed the needs-cherrypick label

stuhood added a commit that referenced this pull request


          Revert "Introduce explicit cache writing job in RscCompile task (#8190)"

33e897e

This reverts commit fed6677.

stuhood mentioned this pull request

Split out a double-check-cache job for jvm/rsc compile #8221

Merged

stuhood added a commit that referenced this pull request


          Split out a double-check-cache job for jvm/rsc compile. (#8221)

e3cf637

### 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


          Split out a double-check-cache job for jvm/rsc compile. (#8221)

71d93e3

### 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

Labels

None yet