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