[engine] no longer content address subject

Review Request #3604 — Created March 24, 2016 and submitted

peiyu
pants
3066, 3090
pants-reviews
kwlzn, patricklaw, stuhood

TL;DR: no longer CA subject, CA nodes but will be smart about it in the next
review.

Still part of [1], at present we CA 100%, subject is sth we can save, because
the only way for a task to view a subject is by Selecting it and in that case
it will be CA-ed as Return(subject).

While we remove subject from CA, because [2] we would have to either make all
subjects orderable (certainly a big restriction and not preferred) or CA the
node itself. This review does the latter. The next review will make CA nodes
(as well as other things) only happen when they are for caching or for
multi-process.

Other changes included:

  • Overwrite default AddressMapper.__repr__, so when it is subject, in debug
    mode, it doesn't have machine address, that results cache misses.
  • Node is content address-ed in Scheduler, which might be questionable.
    Previous thinking is to have scheduler stay out of CA business, this can
    still be achived by having an additional conversion in Engine, we can revisit
    if there is such a need.
  • No more special handling of cyclic Noop, previousely due to scheduler
    introduces a State but it doesn't do CA now it does

[1] Content address performance https://github.com/pantsbuild/pants/issues/3066
[2] Randomness as in https://rbcommons.com/s/twitter/r/3593/

https://travis-ci.org/peiyuwang/pants/builds/118313227

  1. 
      
  2. Print this to stderr.

  3. src/python/pants/engine/exp/scheduler.py (Diff revision 1)
     
     
     

    Note that this is a decision that the engine will make... so I'm not sure it's right for the scheduler to be putting things in storage here. But probably easier to do that bit in another review.

    1. yea, doing this here is only to get this review out. next one will focus on deciding what to CA

  4. Can remove the self.key method probably.

    1. still one place to check the result.key

          self.assertIn((node, self.key(Return(return_value))), dependencies)
      
  5. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Submitted as 899ce539a12baef1342741ac657f565c60cc563b

Loading...