> On Aug 19, 2016, at 7:54 PM, Mandy Chung <mandy.ch...@oracle.com> wrote: > > >> On Aug 19, 2016, at 4:09 PM, Kim Barrett <kim.barr...@oracle.com> wrote: >> >>> On Aug 13, 2016, at 4:07 PM, Mandy Chung <mandy.ch...@oracle.com> wrote: >>> >>> >>>> On Aug 13, 2016, at 1:20 AM, Peter Levart <peter.lev...@gmail.com> wrote: >>>> >>>> Hi Mandy, >>>> >>>> On 08/13/2016 01:55 AM, Mandy Chung wrote: >>>>>> On Aug 8, 2016, at 6:25 PM, Kim Barrett <kim.barr...@oracle.com> >>>>>> wrote: >>>>>> >>>>>> full: >>>>>> http://cr.openjdk.java.net/~kbarrett/8156500/jdk.04/ >>>>>> >>>>>> >>>>>> http://cr.openjdk.java.net/~kbarrett/8156500/hotspot.04/ >>>>> This looks very good. >>>>> >>>>> Have you considered having JVM_WaitForReferencePendingList method to >>>>> return the pending list? i.e. make it a blocking version of >>>>> getAndClearReferencePendingList rather than two operations. >>>>> >>>>> waitForReferenceProcessing is really wait for pending reference(s) >>>>> enqueued (additionally cleaner gets invoked). What do you think to >>>>> rename this method to “waitForPendingReferencesEnqueued”? >>>>> >>>> >>>> I think the split is intentional. It's a clever way to avoid an otherwise >>>> inevitable race that could otherwise cause DBB allocating thread to fail >>>> with OOME prematurely. >>>> >>> >>> Ah I see. I initially read that pendingList doesn’t need to be set within >>> the synchronized block with processPendingActive flag but such race could >>> cause DBB to fail with OOME, as you clarified. >>> >>> Then I suggest waitForReferencePendingList (and its JVM function) be >>> renamed to: >>> boolean awaitReferencePendingList(); >>> >>> a blocking version of "boolean hasReferencePendingList()”. Minor >>> difference but I think it’s little clearer. >> >> Regarding the name, e.g. "waitFor" vs "await", the meaning is the >> same. If there were a documented or defacto preference for "await" I >> would feel obliged to rename, but the JDK seems to use both forms in >> roughly comparable numbers. >> >> Regarding the signature and description, recall that >> waitForReferencePendingList() does not return a value, nor does it >> need to. So documenting it as a blocking version of >> hasReferencePendingList() isn't correct. > > It’s not a suggestion to write “a blocking version of …” in the javadoc (just > my brief way to make my comment that clears my reading of the interfaces) > > If there were multiple reference handling threads, each would want to wait > until there are pending references but only one thread could grab the pending > list. Having it to return a boolean would be useful to me and up to the > callers to decide whether it wants to check the return value (it doesn’t need > to get the pending list if it returns false)
But we don't have multiple reference processing [handling] threads, nor does the proposed design want such. waitForReferencePendingList() is an implementation detail that exists for the exclusive use of the single reference processing thread, to allow it to wait outside the synchronization block where it gets the pending list. The notion of a plurality of callers makes no sense for this function. Maybe I'm not understanding what you are suggesting? It seems like you are talking about some unknown to me design that is different from what I've proposed, and suggesting names and signatures based on that alternative design. > If waitForReferencePendingList returns a boolean, awaitReferencePendingList > reads consistent with “hasReferencePendingList” (one word verb). > >> >>> waitForReferenceProcessing - I’d like the javadoc to include more details >>> or @implNote for example: >>> This method returns true to indicate that some pending references have been >>> enqueued. If there are cleaners, cleaning actions are queued to be >>> invoked e.g. DirectByteBuffer may be deallocated. >>> >>> This would help the readers to understand what this method is intended to >>> do. >> >> I was intentionally a little vague in the description; I don't want to >> be promising too much, and having callers depend on behavior that we >> might want to change. In particular, it was intentional that there's >> no mention of Cleaner or any special behavior for them. >> >> For example, we might later decide we don't want the reference >> processing thread to incur the cost of notifying possible waiters and >> the associated synchronization, and instead let "waiters" return >> immediately when there is reference processing in progress, on the >> assumption that the reference processing thread makes rapid progress. >> In some respects that's probably more similar in effect to what the >> old code did. >> >> Also recall that this function is presently private, accessed by >> nio.Bits code via SharedSecrets (and via reflection by one test). It >> might be appropriate to change the access to this function or provide >> some similar non-private function in the future, at which point the >> javadoc and possibly the semantics and implementation will need to be >> carefully examined. > > This is private API that is subject to change or remove any time. I won’t > worry the javadoc here will give a false impression of its supportedness. > > I think this deserves reasonable level of detailed implementation notes. It > will help the readers to understand what this does rather than reading the > code (both this class and the callsites) carefully with multiple mutators in > mind. > > Mandy
