RFR: 8149793: DirtyCardQueueSet::apply_closure_to_completed_buffer_helper isn't helpful

[Thomas Schatzl]

Hi Kim,

  okay, ship it.

Thanks,
  Thomas

On Di, 2016-02-16 at 21:29 -0500, Kim Barrett wrote:
> > On Feb 16, 2016, at 4:07 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> > 
> > Hi Kim,
> > 
> >  some minor comments about the change:
> > 
> > On Mo, 2016-02-15 at 02:24 -0500, Kim Barrett wrote:
> >> Please review this small cleanup, merging a non-helpful "helper" into
> >> its only caller.
> >> 
> >> CR:
> >> https://bugs.openjdk.java.net/browse/JDK-8149793
> >> 
> >> Webrev:
> >> http://cr.openjdk.java.net/~kbarrett/8149793/webrev.00/
> >> 
> > 
> >  - could the assert be enhanced to print the values of the checked
> > variables to see what actually failed?
> 
> I’m not sure what change you would like.  The assert fails when during_pause
> is true and stop_at is non-zero.  The specific non-zero value doesn’t seem very
> interesting to me here.
> 
> >  - I would prefer if in the if-clause, the seemingly more common case
> > of the buffer should be put first, i.e.
> > 
> > BufferNode* nd = ...;
> > if (nd != NULL) {
> >  // work on buffer
> > } else {
> >  return false;
> > }
> > 
> > like it has been in the original code.
> 
> I like it better with the code for the two cases are both close to the test, rather than
> a big blob of “then” code between the test and the “else”.  Also, a later cleanup
> in my queue changes it to
> 
>   if (nd == NULL) {
>     …;
>   } else if (apply_closure_to_buffer(cl, nd, ...) {
>     …;
>   } else {
>     enqueue_complete_buffer(nd);
>     return false;
>   }
> 
>